Skip to content
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

[RISCV] Use LiveIntervals::extendToIndices to extend AVL in insertVSETVLI #97512

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jul 3, 2024

In #96200 we handled extending AVL LiveIntervals across basic blocks, which fixed a crash in a test case in 133ab9a.

This was done by manually adding a single segment to the LiveInterval to extend it from AVL def -> inserted vsetvli, but in hindsight this was too simple and fails to handle cases where the vsetlvi is located before the AVL def.

This patch fixes this by using LiveIntervals::extendToIndices instead which can handle these cases.

(The crash that this fixes is separate from the crash in #97264)

…TVLI

In llvm#96200 we handled extending AVL LiveIntervals across basic blocks, which fixed a crash in a test case in 133ab9a.

This was done by manually adding a single segment to the LiveInterval to extend it from AVL def -> inserted vsetvli, but in hindsight this was too simple and fails to handle cases where the vsetlvi is located before the AVL def.

This patch fixes this by using LiveIntervals::extendToIndices instead which can handle these cases.

(The crash that this fixes is separate from the crash in llvm#97264)
@llvmbot
Copy link
Member

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Luke Lau (lukel97)

Changes

In #96200 we handled extending AVL LiveIntervals across basic blocks, which fixed a crash in a test case in 133ab9a.

This was done by manually adding a single segment to the LiveInterval to extend it from AVL def -> inserted vsetvli, but in hindsight this was too simple and fails to handle cases where the vsetlvi is located before the AVL def.

This patch fixes this by using LiveIntervals::extendToIndices instead which can handle these cases.

(The crash that this fixes is separate from the crash in #97264)


Full diff: https://github.com/llvm/llvm-project/pull/97512.diff

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp (+1-4)
  • (modified) llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll (+39-9)
diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index 7e6eef47c121c..a1ae8a1250813 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -1161,10 +1161,7 @@ void RISCVInsertVSETVLI::insertVSETVLI(MachineBasicBlock &MBB,
     // isn't always the case, e.g. PseudoVMV_X_S doesn't have an AVL operand or
     // we've taken the AVL from the VL output of another vsetvli.
     LiveInterval &LI = LIS->getInterval(AVLReg);
-    // Need to get non-const VNInfo
-    VNInfo *VNI = LI.getValNumInfo(Info.getAVLVNInfo()->id);
-    LI.addSegment(LiveInterval::Segment(
-        VNI->def, LIS->getInstructionIndex(*MI).getRegSlot(), VNI));
+    LIS->extendToIndices(LI, {LIS->getInstructionIndex(*MI).getRegSlot()});
   }
 }
 
diff --git a/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll b/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
index 884c756840eb9..d2d425215ea9f 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
@@ -322,19 +322,19 @@ declare void @foo()
 define <vscale x 1 x double> @test8(i64 %avl, i8 zeroext %cond, <vscale x 1 x double> %a, <vscale x 1 x double> %b) nounwind {
 ; CHECK-LABEL: test8:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    addi sp, sp, -32
-; CHECK-NEXT:    sd ra, 24(sp) # 8-byte Folded Spill
-; CHECK-NEXT:    sd s0, 16(sp) # 8-byte Folded Spill
-; CHECK-NEXT:    csrr a2, vlenb
-; CHECK-NEXT:    slli a2, a2, 1
-; CHECK-NEXT:    sub sp, sp, a2
-; CHECK-NEXT:    mv s0, a0
 ; CHECK-NEXT:    vsetvli zero, a0, e64, m1, ta, ma
 ; CHECK-NEXT:    beqz a1, .LBB6_2
 ; CHECK-NEXT:  # %bb.1: # %if.then
 ; CHECK-NEXT:    vfadd.vv v8, v8, v9
-; CHECK-NEXT:    j .LBB6_3
+; CHECK-NEXT:    ret
 ; CHECK-NEXT:  .LBB6_2: # %if.else
+; CHECK-NEXT:    addi sp, sp, -32
+; CHECK-NEXT:    sd ra, 24(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    sd s0, 16(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    csrr a1, vlenb
+; CHECK-NEXT:    slli a1, a1, 1
+; CHECK-NEXT:    sub sp, sp, a1
+; CHECK-NEXT:    mv s0, a0
 ; CHECK-NEXT:    csrr a0, vlenb
 ; CHECK-NEXT:    add a0, a0, sp
 ; CHECK-NEXT:    addi a0, a0, 16
@@ -350,7 +350,6 @@ define <vscale x 1 x double> @test8(i64 %avl, i8 zeroext %cond, <vscale x 1 x do
 ; CHECK-NEXT:    vl1r.v v9, (a0) # Unknown-size Folded Reload
 ; CHECK-NEXT:    vsetvli zero, s0, e64, m1, ta, ma
 ; CHECK-NEXT:    vfsub.vv v8, v9, v8
-; CHECK-NEXT:  .LBB6_3: # %if.then
 ; CHECK-NEXT:    csrr a0, vlenb
 ; CHECK-NEXT:    slli a0, a0, 1
 ; CHECK-NEXT:    add sp, sp, a0
@@ -1063,6 +1062,37 @@ exit:
   ret <vscale x 2 x i32> %c
 }
 
+define void @cross_block_avl_extend_backwards(i1 %cond, <vscale x 8 x i8> %v, ptr %p, i64 %avl) {
+; CHECK-LABEL: extend_avl_backwards:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a0, a0, 1
+; CHECK-NEXT:    beqz a0, .LBB26_2
+; CHECK-NEXT:  # %bb.1: # %exit
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB26_2: # %bar
+; CHECK-NEXT:    addi a2, a2, 1
+; CHECK-NEXT:  .LBB26_3: # %foo
+; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    vsetivli zero, 1, e8, m1, ta, ma
+; CHECK-NEXT:    vse8.v v8, (a1)
+; CHECK-NEXT:    vsetvli zero, a2, e8, m1, ta, ma
+; CHECK-NEXT:    vse8.v v8, (a1)
+; CHECK-NEXT:    j .LBB26_3
+entry:
+  br i1 %cond, label %exit, label %bar
+foo:
+  ; Force a vl toggle
+  call void @llvm.riscv.vse.nxv8i8.i64(<vscale x 8 x i8> %v, ptr %p, i64 1)
+  ; %add's LiveRange needs to be extended backwards to here.
+  call void @llvm.riscv.vse.nxv8i8.i64(<vscale x 8 x i8> %v, ptr %p, i64 %add)
+  br label %foo
+exit:
+  ret void
+bar:
+  %add = add i64 %avl, 1
+  br label %foo
+}
+
 define void @vlmax_avl_phi(i1 %cmp, ptr %p, i64 %a, i64 %b) {
 ; CHECK-LABEL: vlmax_avl_phi:
 ; CHECK:       # %bb.0: # %entry

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I forgot to originally after renaming and moving the test
@lukel97 lukel97 merged commit b3be148 into llvm:main Jul 3, 2024
4 of 6 checks passed
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…TVLI (llvm#97512)

In llvm#96200 we handled extending AVL LiveIntervals across basic blocks,
which fixed a crash in a test case in
133ab9a.

This was done by manually adding a single segment to the LiveInterval to
extend it from AVL def -> inserted vsetvli, but in hindsight this was
too simple and fails to handle cases where the vsetlvi is located before
the AVL def.

This patch fixes this by using LiveIntervals::extendToIndices instead
which can handle these cases.

(The crash that this fixes is separate from the crash in llvm#97264)
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
…TVLI (llvm#97512)

In llvm#96200 we handled extending AVL LiveIntervals across basic blocks,
which fixed a crash in a test case in
133ab9a.

This was done by manually adding a single segment to the LiveInterval to
extend it from AVL def -> inserted vsetvli, but in hindsight this was
too simple and fails to handle cases where the vsetlvi is located before
the AVL def.

This patch fixes this by using LiveIntervals::extendToIndices instead
which can handle these cases.

(The crash that this fixes is separate from the crash in llvm#97264)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants