[RISCV][VLOPT] Fix assertion failure across blocks #124734
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-risc-v Author: Luke Lau (lukel97) ChangesWhilst adding a cross-block test, I encountered an assertion failure in the second pass where we check the instruction popped off the worklist is a candidate. The leaf instruction %c in this case will be added to the worklist when its VL is VLMAX, but during the first pass it will have its VL reduced to 1. Then in the second pass when its processed via the worklist, isCandidate will no longer be true due to its VL == 1. I think the easiest fix for this is to remove the VL > 1 check in isCandidate, so now it just checks static properties about the MCInstrDesc. Full diff: https://github.com/llvm/llvm-project/pull/124734.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
index e8c01e57038bfb..9bd3895489da10 100644
--- a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
@@ -1143,16 +1143,6 @@ bool RISCVVLOptimizer::isCandidate(const MachineInstr &MI) const {
if (MI.getNumDefs() != 1)
return false;
- unsigned VLOpNum = RISCVII::getVLOpNum(Desc);
- const MachineOperand &VLOp = MI.getOperand(VLOpNum);
-
- // If the VL is 1, then there is no need to reduce it. This is an
- // optimization, not needed to preserve correctness.
- if (VLOp.isImm() && VLOp.getImm() == 1) {
- LLVM_DEBUG(dbgs() << " Not a candidate because VL is already 1\n");
- return false;
- }
-
if (MI.mayRaiseFPException()) {
LLVM_DEBUG(dbgs() << "Not a candidate because may raise FP exception\n");
return false;
diff --git a/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
index 027eb8ca3c17f0..7a28eaaaa5d8be 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
+++ b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
@@ -149,3 +149,49 @@ body: |
; CHECK-NEXT: early-clobber %y:vrm2 = PseudoVWADD_WV_M1_TIED $noreg, %x, 1, 3 /* e8 */, 0 /* tu, mu */
%x:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, -1, 3 /* e8 */, 0 /* tu, mu */
%y:vrm2 = PseudoVWADD_WV_M1_TIED $noreg, %x, 1, 3 /* e8 */, 0 /* tu, mu */
+...
+---
+name: crossbb
+body: |
+ ; CHECK-LABEL: name: crossbb
+ ; CHECK: bb.0:
+ ; CHECK-NEXT: successors: %bb.3(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: PseudoBR %bb.3
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1:
+ ; CHECK-NEXT: %a1:vr = PseudoVADD_VV_M1 $noreg, %c, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ ; CHECK-NEXT: %a2:vr = PseudoVADD_VV_M1 $noreg, %a1, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ ; CHECK-NEXT: $v8 = COPY %a2
+ ; CHECK-NEXT: PseudoRET
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ ; CHECK-NEXT: %b1:vr = PseudoVADD_VV_M1 $noreg, %c, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ ; CHECK-NEXT: %b2:vr = PseudoVADD_VV_M1 $noreg, %b1, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ ; CHECK-NEXT: $v8 = COPY %b2
+ ; CHECK-NEXT: PseudoRET
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.3:
+ ; CHECK-NEXT: successors: %bb.1(0x40000000), %bb.2(0x40000000)
+ ; CHECK-NEXT: liveins: $x1
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: %c:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ ; CHECK-NEXT: BEQ $x1, $x0, %bb.1
+ ; CHECK-NEXT: PseudoBR %bb.2
+ bb.0:
+ PseudoBR %bb.3
+ bb.1:
+ %a1:vr = PseudoVADD_VV_M1 $noreg, %c, $noreg, -1, 3 /* e8 */, 0 /* tu, mu */
+ %a2:vr = PseudoVADD_VV_M1 $noreg, %a1, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ $v8 = COPY %a2
+ PseudoRET
+ bb.2:
+ %b1:vr = PseudoVADD_VV_M1 $noreg, %c, $noreg, -1, 3 /* e8 */, 0 /* tu, mu */
+ %b2:vr = PseudoVADD_VV_M1 $noreg, %b1, $noreg, 1, 3 /* e8 */, 0 /* tu, mu */
+ $v8 = COPY %b2
+ PseudoRET
+ bb.3:
+ liveins: $x1
+ %c:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, -1, 3 /* e8 */, 0 /* tu, mu */
+ BEQ $x1, $x0, %bb.1
+ PseudoBR %bb.2
|
Whilst adding a cross-block test, I encountered an assertion failure in the second pass where we check the instruction popped off the worklist is a candidate. The leaf instruction %c in this case will be added to the worklist when its VL is VLMAX, but during the first pass it will have its VL reduced to 1. Then in the second pass when its processed via the worklist, isCandidate will no longer be true due to its VL == 1. I think the easiest fix for this is to remove the VL > 1 check in isCandidate, so now it just checks static properties about the MCInstrDesc.
|
I'm not sure I agree with this approach. I think there is a deeper problem that is going on: something is on the worklist that is not a candidate, and this is an invariant we are currently not permitting. I think we should handle it in one of two ways:
I think (1) is probably easier, but I don't have a strong preference. |
The issue lies in that a candidate can become a non-candidate whilst on the worklist, because after an instruction gets reduced it's no longer a candidate, which can happen before it's popped off the worklist.
I think we can do this approach. It's probably worth noting that there's two types of check going on in isCandidate at the moment, those to ensure correctness (isSupportedInstr, mayRaiseFPException), and those to see if it's worthwhile (VL > 1). We could potentially move the latter into tryReduceVL to keep it alongside the other VL checks. |
lukel97
force-pushed
the
vloptimizer/crossbb
branch
from
cabc640 to
4ca7c87
Compare
This seems like the right approach to me. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/14814 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Whilst adding a cross-block test, I encountered an assertion failure in the second pass where we check the instruction popped off the worklist is a candidate.
The leaf instruction %c in this case will be added to the worklist when its VL is VLMAX, but during the first pass it will have its VL reduced to 1.
Then in the second pass when its processed via the worklist, isCandidate will no longer be true due to its VL == 1.
This fixes it by moving the VL == 1 check to tryReduceVL, keeping it alongside the other VL check for bailing out early as an optimisation.