-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[RISCV] Fix crash when trying to remove segment #146524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-backend-risc-v Author: None (sc-clulzze) ChangesFIxes #146518 Full diff: https://github.com/llvm/llvm-project/pull/146524.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index 78d64ea67324f..bc4bf94d16042 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -1768,8 +1768,9 @@ void RISCVInsertVSETVLI::insertReadVL(MachineBasicBlock &MBB) {
SlotIndex NewDefSI =
LIS->InsertMachineInstrInMaps(*ReadVLMI).getRegSlot();
LiveInterval &DefLI = LIS->getInterval(VLOutput);
- VNInfo *DefVNI = DefLI.getVNInfoAt(DefLI.beginIndex());
- DefLI.removeSegment(DefLI.beginIndex(), NewDefSI);
+ const auto *DefSeg = DefLI.getSegmentContaining(NewDefSI);
+ VNInfo *DefVNI = DefLI.getVNInfoAt(DefSeg->start);
+ DefLI.removeSegment(DefSeg->start, NewDefSI);
DefVNI->def = NewDefSI;
}
}
diff --git a/llvm/test/CodeGen/RISCV/rvv/vleff-crash.ll b/llvm/test/CodeGen/RISCV/rvv/vleff-crash.ll
new file mode 100644
index 0000000000000..0da62d5a80046
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/vleff-crash.ll
@@ -0,0 +1,58 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=riscv64 -mattr=+zve64d,+f,+d,+zfh,+zvfh -verify-machineinstrs < %s | FileCheck %s
+
+define i64 @strlen16_vec(ptr %s) {
+; CHECK-LABEL: strlen16_vec:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: li a1, 0
+; CHECK-NEXT: li a2, 16
+; CHECK-NEXT: li a3, -1
+; CHECK-NEXT: li a4, 16
+; CHECK-NEXT: j .LBB0_2
+; CHECK-NEXT: .LBB0_1: # %while.body
+; CHECK-NEXT: # in Loop: Header=BB0_2 Depth=1
+; CHECK-NEXT: add a1, a6, a1
+; CHECK-NEXT: bne a5, a3, .LBB0_5
+; CHECK-NEXT: .LBB0_2: # %while.cond
+; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT: bne a4, a2, .LBB0_5
+; CHECK-NEXT: # %bb.3: # %while.body
+; CHECK-NEXT: # in Loop: Header=BB0_2 Depth=1
+; CHECK-NEXT: vsetivli zero, 16, e16, m1, ta, ma
+; CHECK-NEXT: vle16ff.v v8, (a0)
+; CHECK-NEXT: csrr a4, vl
+; CHECK-NEXT: vmseq.vi v8, v8, 0
+; CHECK-NEXT: vfirst.m a5, v8
+; CHECK-NEXT: mv a6, a4
+; CHECK-NEXT: beq a5, a3, .LBB0_1
+; CHECK-NEXT: # %bb.4: # %while.body
+; CHECK-NEXT: # in Loop: Header=BB0_2 Depth=1
+; CHECK-NEXT: mv a6, a5
+; CHECK-NEXT: j .LBB0_1
+; CHECK-NEXT: .LBB0_5: # %while.end
+; CHECK-NEXT: mv a0, a1
+; CHECK-NEXT: ret
+entry:
+ br label %while.cond
+
+while.cond: ; preds = %while.body, %entry
+ %new_vl.0 = phi i64 [ 16, %entry ], [ %2, %while.body ]
+ %len.0 = phi i64 [ 0, %entry ], [ %len.1, %while.body ]
+ %cmp = icmp eq i64 %new_vl.0, 16
+ br i1 %cmp, label %while.body, label %while.end
+
+while.body: ; preds = %while.cond
+ %0 = tail call { <vscale x 4 x i16>, i64 } @llvm.riscv.vleff.nxv4i16.i64(<vscale x 4 x i16> poison, ptr %s, i64 16)
+ %1 = extractvalue { <vscale x 4 x i16>, i64 } %0, 0
+ %2 = extractvalue { <vscale x 4 x i16>, i64 } %0, 1
+ %3 = tail call <vscale x 4 x i1> @llvm.riscv.vmseq.nxv4i16.i16.i64(<vscale x 4 x i16> %1, i16 0, i64 %2)
+ %4 = tail call i64 @llvm.riscv.vfirst.nxv4i1.i64(<vscale x 4 x i1> %3, i64 %2)
+ %cmp1 = icmp eq i64 %4, -1
+ %.13 = select i1 %cmp1, i64 %2, i64 %4
+ %len.1 = add i64 %.13, %len.0
+ br i1 %cmp1, label %while.cond, label %while.end
+
+while.end: ; preds = %while.body, %while.cond
+ %len.2 = phi i64 [ %len.1, %while.body ], [ %len.0, %while.cond ]
+ ret i64 %len.2
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, can you create an MIR test in vsetvli-insert.mir that shows this?
Also would be good to clarify in the title under what exact conditions this crash happens and how this fixes it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into vsetvli-insert.ll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to vsetvli-insert.ll
while.body: ; preds = %while.cond | ||
%0 = tail call { <vscale x 4 x i16>, i64 } @llvm.riscv.vleff.nxv4i16.i64(<vscale x 4 x i16> poison, ptr %s, i64 16) | ||
%1 = extractvalue { <vscale x 4 x i16>, i64 } %0, 0 | ||
%2 = extractvalue { <vscale x 4 x i16>, i64 } %0, 1 | ||
%3 = tail call <vscale x 4 x i1> @llvm.riscv.vmseq.nxv4i16.i16.i64(<vscale x 4 x i16> %1, i16 0, i64 %2) | ||
%4 = tail call i64 @llvm.riscv.vfirst.nxv4i1.i64(<vscale x 4 x i1> %3, i64 %2) | ||
%cmp1 = icmp eq i64 %4, -1 | ||
%.13 = select i1 %cmp1, i64 %2, i64 %4 | ||
%len.1 = add i64 %.13, %len.0 | ||
br i1 %cmp1, label %while.cond, label %while.end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the crash reproducable with only the while.body block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reduced the test a bit, but could not get rid of while.cond block
3196544
to
c1b6e12
Compare
I think it would still be good to have an MIR test if you can add one |
Added this test to vsetvli-insert.mir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks, just some nits
... | ||
--- | ||
name: vsetvli_vleff | ||
alignment: 4 | ||
tracksRegLiveness: true | ||
registers: | ||
- { id: 0, class: gpr, preferred-register: '' } | ||
- { id: 1, class: gpr, preferred-register: '' } | ||
- { id: 2, class: gpr, preferred-register: '' } | ||
- { id: 3, class: gpr, preferred-register: '' } | ||
- { id: 4, class: gpr, preferred-register: '' } | ||
- { id: 5, class: vr, preferred-register: '%7' } | ||
- { id: 6, class: gpr, preferred-register: '' } | ||
- { id: 7, class: vr, preferred-register: '%5' } | ||
- { id: 8, class: vr, preferred-register: '' } | ||
- { id: 9, class: gpr, preferred-register: '' } | ||
frameInfo: | ||
maxAlignment: 1 | ||
machineFunctionInfo: {} | ||
body: | | ||
; CHECK-LABEL: name: vsetvli_vleff | ||
; CHECK: bb.0.entry: | ||
; CHECK-NEXT: successors: %bb.1(0x80000000) | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x0 | ||
; CHECK-NEXT: dead [[PseudoVSETVLIX0_:%[0-9]+]]:gprnox0 = PseudoVSETVLIX0 killed $x0, 200 /* e16, m1, ta, ma */, implicit-def $vl, implicit-def $vtype | ||
; CHECK-NEXT: renamable $v8 = PseudoVMV_V_I_M1 undef renamable $v8, 0, -1, 4 /* e16 */, 0 /* tu, mu */, implicit $vl, implicit $vtype | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: bb.1.while.cond: | ||
; CHECK-NEXT: successors: %bb.2(0x7c000000), %bb.3(0x04000000) | ||
; CHECK-NEXT: liveins: $v8 | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: BNE [[COPY]], $x0, %bb.3 | ||
; CHECK-NEXT: PseudoBR %bb.2 | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: bb.2.while.body: | ||
; CHECK-NEXT: successors: %bb.1(0x80000000) | ||
; CHECK-NEXT: liveins: $v8 | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: $x0 = PseudoVSETIVLI 0, 136 /* e16, m1, tu, ma */, implicit-def $vl, implicit-def $vtype | ||
; CHECK-NEXT: renamable $v9 = COPY renamable $v8, implicit $vtype | ||
; CHECK-NEXT: dead renamable $v9, $x0 = PseudoVLE16FF_V_M1 killed renamable $v9, $x0, 0, 4 /* e16 */, 2 /* tu, ma */, implicit-def dead $vl, implicit $vl, implicit $vtype :: (load unknown-size from `ptr null`, align 2) | ||
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = PseudoReadVL implicit $vl | ||
; CHECK-NEXT: PseudoBR %bb.1 | ||
; CHECK-NEXT: {{ $}} | ||
; CHECK-NEXT: bb.3.while.end: | ||
; CHECK-NEXT: $x10 = COPY $x0 | ||
; CHECK-NEXT: PseudoRET implicit killed $x10 | ||
bb.0.entry: | ||
successors: %bb.1(0x80000000) | ||
%9:gpr = COPY $x0 | ||
renamable $v8 = PseudoVMV_V_I_M1 undef renamable $v8, 0, -1, 4 /* e16 */, 0 /* tu, mu */ | ||
bb.1.while.cond: | ||
successors: %bb.2(0x7c000000), %bb.3(0x04000000) | ||
liveins: $v8 | ||
BNE %9, $x0, %bb.3 | ||
PseudoBR %bb.2 | ||
bb.2.while.body: | ||
successors: %bb.1(0x80000000) | ||
liveins: $v8 | ||
renamable $v9 = COPY renamable $v8 | ||
dead renamable $v9, %9:gpr = PseudoVLE16FF_V_M1 killed renamable $v9, $x0, 0, 4 /* e16 */, 2 /* tu, ma */, implicit-def dead $vl :: (load unknown-size from `ptr null`, align 2) | ||
PseudoBR %bb.1 | ||
bb.3.while.end: | ||
$x10 = COPY $x0 | ||
PseudoRET implicit killed $x10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be made more minimal, something like this should reproduce the crash?
---
name: vsetvli_vleff
tracksRegLiveness: true
body: |
bb.0:
PseudoBR %bb.2
bb.1:
$x8 = COPY %vl
PseudoRET implicit $x8
bb.2:
$noreg, %vl:gpr = PseudoVLE16FF_V_M1 $noreg, $noreg, 0, 4 /* e16 */, 2 /* tu, ma */
PseudoBR %bb.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crash does reproduce on this MIR, but you can write an exact IR for it since bb.0 is not a predecessor of bb.1. So I've modified it a little bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you can just leave the IR as "ret void" which lets you write whatever for the MIR
@@ -1768,8 +1768,9 @@ void RISCVInsertVSETVLI::insertReadVL(MachineBasicBlock &MBB) { | |||
SlotIndex NewDefSI = | |||
LIS->InsertMachineInstrInMaps(*ReadVLMI).getRegSlot(); | |||
LiveInterval &DefLI = LIS->getInterval(VLOutput); | |||
VNInfo *DefVNI = DefLI.getVNInfoAt(DefLI.beginIndex()); | |||
DefLI.removeSegment(DefLI.beginIndex(), NewDefSI); | |||
const auto *DefSeg = DefLI.getSegmentContaining(NewDefSI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
const auto *DefSeg = DefLI.getSegmentContaining(NewDefSI); | |
LiveRange::Segment *DefSeg = DefLI.getSegmentContaining(NewDefSI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
br i1 %cmp, label %while.body, label %while.end | ||
|
||
while.body: | ||
%0 = tail call { <vscale x 4 x i16>, i64 } @llvm.riscv.vleff.nxv4i16.i64(<vscale x 4 x i16> zeroinitializer, ptr null, i64 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pass in a pointer + evl operand to this test just so the vleff doesn't get optimised away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
bb0: | ||
br i1 poison, label %bb1, label %bb2 | ||
bb1: | ||
%0 = tail call { <vscale x 4 x i16>, i64 } @llvm.riscv.vleff.nxv4i16.i64(<vscale x 4 x i16> zeroinitializer, ptr null, i64 0) | ||
%1 = extractvalue { <vscale x 4 x i16>, i64 } %0, 1 | ||
br label %bb2 | ||
bb2: | ||
%x8 = phi i64 [%1, %bb1], [0, %bb0] | ||
ret i64 %x8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bb0: | |
br i1 poison, label %bb1, label %bb2 | |
bb1: | |
%0 = tail call { <vscale x 4 x i16>, i64 } @llvm.riscv.vleff.nxv4i16.i64(<vscale x 4 x i16> zeroinitializer, ptr null, i64 0) | |
%1 = extractvalue { <vscale x 4 x i16>, i64 } %0, 1 | |
br label %bb2 | |
bb2: | |
%x8 = phi i64 [%1, %bb1], [0, %bb0] | |
ret i64 %x8 | |
ret void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
; CHECK-NEXT: bb.2.bb2: | ||
; CHECK-NEXT: $x10 = COPY [[COPY]] | ||
; CHECK-NEXT: PseudoRET implicit killed $x10 | ||
bb.0.bb0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bb.0.bb0: | |
bb.0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
BNE $x0, $x0, %bb.2 | ||
PseudoBR %bb.1 | ||
bb.1.bb1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bb.1.bb1: | |
bb.1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
renamable $v8 = PseudoVMV_V_I_M1 undef renamable $v8, 0, -1, 4 /* e16 */, 0 /* tu, mu */ | ||
dead renamable $v8, %9:gpr = PseudoVLE16FF_V_M1 killed renamable $v8, $x0, 0, 4 /* e16 */, 2 /* tu, ma */, implicit-def dead $vl :: (load unknown-size from `ptr null`, align 2) | ||
bb.2.bb2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bb.2.bb2: | |
bb.2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
renamable $v8 = PseudoVMV_V_I_M1 undef renamable $v8, 0, -1, 4 /* e16 */, 0 /* tu, mu */ | ||
dead renamable $v8, %9:gpr = PseudoVLE16FF_V_M1 killed renamable $v8, $x0, 0, 4 /* e16 */, 2 /* tu, ma */, implicit-def dead $vl :: (load unknown-size from `ptr null`, align 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamable $v8 = PseudoVMV_V_I_M1 undef renamable $v8, 0, -1, 4 /* e16 */, 0 /* tu, mu */ | |
dead renamable $v8, %9:gpr = PseudoVLE16FF_V_M1 killed renamable $v8, $x0, 0, 4 /* e16 */, 2 /* tu, ma */, implicit-def dead $vl :: (load unknown-size from `ptr null`, align 2) | |
$noreg, %vl:gpr = PseudoVLE16FF_V_M1 $noreg, $noreg, 0, 4 /* e16 */, 2 /* tu, ma */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
$x10 = COPY %9 | ||
PseudoRET implicit killed $x10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$x10 = COPY %9 | |
PseudoRET implicit killed $x10 | |
$x10 = COPY %vl | |
PseudoRET implicit killed $x10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
bb.0.bb0: | ||
successors: %bb.1(0x40000000), %bb.2(0x40000000) | ||
%9:gpr = COPY $x0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%9:gpr = COPY $x0 | |
%vl:gpr = COPY $x0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
@sc-clulzze Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Thanks for fixing this, I've gone ahead and committed it if that's ok, so it should be in for LLVM 21 |
LiveInterval DefLI can be consisting of multiple segments, and SlotIndex NewDefSI can be inside any of them. Currenty it is assumed that NewDefSI belongs to the first segment, and when trying to call removeSegment, clang crashes since there is no segment containing [DefLI.beginIndex(), NewDefSI]
FIxes #146518