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] Return nullptr for PHI defs in VSETVLIInfo::getAVLDefMI #97395

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jul 2, 2024

When checking if a VSETVLIInfo is compatible, we call hasEquallyZeroAVL when only the AVL-zeroness is demanded. This will try to lookup the defining MachineInstr (to check if it's an ADDI immediate) via getAVLDefMI, but in it we were asserting that the VSETVLIInfo's AVL wouldn't come from a phi. It turns out this can happen in normal circumstances.

This causes a crash when compiling highway, so this fixes it by relaxing the assertion.

When checking if a VSETVLIInfo is compatible, we call hasEquallyZeroAVL when only the AVL-zeroness is demanded. This will try to lookup the defining MachineInstr (to check if it's an ADDI immediate) via getAVLDefMI, but in it we were asserting that the VSETVLIInfo's AVL wouldn't come from a phi. It turns out this can happen in normal circumstances.

This causes a crash when compiling highway, so this fixes it by relaxing the assertion.
@llvmbot
Copy link
Member

llvmbot commented Jul 2, 2024

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

Author: Luke Lau (lukel97)

Changes

When checking if a VSETVLIInfo is compatible, we call hasEquallyZeroAVL when only the AVL-zeroness is demanded. This will try to lookup the defining MachineInstr (to check if it's an ADDI immediate) via getAVLDefMI, but in it we were asserting that the VSETVLIInfo's AVL wouldn't come from a phi. It turns out this can happen in normal circumstances.

This causes a crash when compiling highway, so this fixes it by relaxing the assertion.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp (+4-3)
  • (modified) llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll (+27)
diff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index bf693344b070a..7e6eef47c121c 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -586,13 +586,14 @@ class VSETVLIInfo {
   }
   // Most AVLIsReg infos will have a single defining MachineInstr, unless it was
   // a PHI node. In that case getAVLVNInfo()->def will point to the block
-  // boundary slot.  If LiveIntervals isn't available, then nullptr is returned.
+  // boundary slot and this will return nullptr.  If LiveIntervals isn't
+  // available, nullptr is also returned.
   const MachineInstr *getAVLDefMI(const LiveIntervals *LIS) const {
     assert(hasAVLReg());
-    if (!LIS)
+    if (!LIS || getAVLVNInfo()->isPHIDef())
       return nullptr;
     auto *MI = LIS->getInstructionFromIndex(getAVLVNInfo()->def);
-    assert(!(getAVLVNInfo()->isPHIDef() && MI));
+    assert(MI);
     return MI;
   }
 
diff --git a/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll b/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
index 68e8f5dd0a406..92c8f9611db49 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
@@ -1062,3 +1062,30 @@ exit:
   %c = call <vscale x 2 x i32> @llvm.riscv.vadd.nxv2i32(<vscale x 2 x i32> undef, <vscale x 2 x i32> %a, <vscale x 2 x i32> %d, i64 %vl)
   ret <vscale x 2 x i32> %c
 }
+
+define void @vlmax_avl_phi(i1 %cmp, ptr %p) {
+; CHECK-LABEL: vlmax_avl_phi:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    andi a0, a0, 1
+; CHECK-NEXT:    vsetvli a0, zero, e8, m1, ta, ma
+; CHECK-NEXT:    vmv.v.i v8, 0
+; CHECK-NEXT:    vsetivli zero, 1, e8, m1, ta, ma
+; CHECK-NEXT:    vse8.v v8, (a1)
+; CHECK-NEXT:    ret
+entry:
+  br i1 %cmp, label %foo, label %bar
+
+foo:
+  %vl.foo = tail call i64 @llvm.riscv.vsetvlimax.i64(i64 0, i64 0)
+  br label %exit
+
+bar:
+  %vl.bar = tail call i64 @llvm.riscv.vsetvlimax.i64(i64 0, i64 0)
+  br label %exit
+
+exit:
+  %phivl = phi i64 [ %vl.foo, %foo ], [ %vl.bar, %bar ]
+  %1 = tail call <vscale x 8 x i8> @llvm.riscv.vmv.v.x.nxv8i8.i64(<vscale x 8 x i8> poison, i8 0, i64 %phivl)
+  call void @llvm.riscv.vse.nxv8i8(<vscale x 8 x i8> %1, ptr %p, i64 1)
+  ret void
+}

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.

@lukel97 lukel97 merged commit 62a967d into llvm:main Jul 2, 2024
5 of 7 checks passed
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…#97395)

When checking if a VSETVLIInfo is compatible, we call hasEquallyZeroAVL
if only the AVL-zeroness is demanded. This will try to lookup the
defining MachineInstr (to check if it's an ADDI immediate) via
getAVLDefMI, but in it we were asserting that the VSETVLIInfo's AVL
wouldn't come from a phi. It turns out this can happen in normal
circumstances.

This causes a crash when compiling highway, so this fixes it by relaxing
the assertion.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
…#97395)

When checking if a VSETVLIInfo is compatible, we call hasEquallyZeroAVL
if only the AVL-zeroness is demanded. This will try to lookup the
defining MachineInstr (to check if it's an ADDI immediate) via
getAVLDefMI, but in it we were asserting that the VSETVLIInfo's AVL
wouldn't come from a phi. It turns out this can happen in normal
circumstances.

This causes a crash when compiling highway, so this fixes it by relaxing
the assertion.
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