[RISCV] Fix same mask vmerge peephole discarding false operand #107827
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
This fixes the issue raised in llvm#106108 (comment) True's passthru needs to be equivalent to vmerge's false, but we also allow true's passthru to be undef. However if it's undef then we need to replace it with vmerge's false, otherwise we end up discarding the false operand entirely. The changes in fixed-vectors-strided-load-store-asm.ll undo the changes in llvm#106108 where we introduced this miscompile.
|
@llvm/pr-subscribers-backend-risc-v Author: Luke Lau (lukel97) ChangesThis fixes the issue raised in #106108 (comment) True's passthru needs to be equivalent to vmerge's false, but we also allow true's passthru to be undef. However if it's undef then we need to replace it with false, otherwise we end up discarding the false operand entirely. The changes in fixed-vectors-strided-load-store-asm.ll undo the changes in #106108 where we introduced this miscompile. Full diff: https://github.com/llvm/llvm-project/pull/107827.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
index 48ea2a01876177..1bad53f807546c 100644
--- a/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVectorPeephole.cpp
@@ -66,7 +66,7 @@ class RISCVVectorPeephole : public MachineFunctionPass {
bool convertToWholeRegister(MachineInstr &MI) const;
bool convertToUnmasked(MachineInstr &MI) const;
bool convertAllOnesVMergeToVMv(MachineInstr &MI) const;
- bool convertSameMaskVMergeToVMv(MachineInstr &MI) const;
+ bool convertSameMaskVMergeToVMv(MachineInstr &MI);
bool foldUndefPassthruVMV_V_V(MachineInstr &MI);
bool foldVMV_V_V(MachineInstr &MI);
@@ -396,7 +396,7 @@ bool RISCVVectorPeephole::convertAllOnesVMergeToVMv(MachineInstr &MI) const {
/// ->
/// %true = PseudoVADD_VV_M1_MASK %false, %x, %y, %mask, vl1, sew, policy
/// %x = PseudoVMV_V_V %passthru, %true, vl2, sew, tu_mu
-bool RISCVVectorPeephole::convertSameMaskVMergeToVMv(MachineInstr &MI) const {
+bool RISCVVectorPeephole::convertSameMaskVMergeToVMv(MachineInstr &MI) {
unsigned NewOpc = getVMV_V_VOpcodeForVMERGE_VVM(MI);
if (!NewOpc)
return false;
@@ -405,18 +405,24 @@ bool RISCVVectorPeephole::convertSameMaskVMergeToVMv(MachineInstr &MI) const {
!hasSameEEW(MI, *True))
return false;
- // True's passthru needs to be equivalent to False
- Register TruePassthruReg = True->getOperand(1).getReg();
- Register FalseReg = MI.getOperand(2).getReg();
- if (TruePassthruReg != RISCV::NoRegister && TruePassthruReg != FalseReg)
- return false;
-
const MachineInstr *TrueV0Def = V0Defs.lookup(True);
const MachineInstr *MIV0Def = V0Defs.lookup(&MI);
assert(TrueV0Def && TrueV0Def->isCopy() && MIV0Def && MIV0Def->isCopy());
if (TrueV0Def->getOperand(1).getReg() != MIV0Def->getOperand(1).getReg())
return false;
+ // True's passthru needs to be equivalent to False
+ Register TruePassthruReg = True->getOperand(1).getReg();
+ Register FalseReg = MI.getOperand(2).getReg();
+ if (TruePassthruReg != FalseReg) {
+ // If True's passthru is undef see if we can change it to False
+ if (TruePassthruReg != RISCV::NoRegister ||
+ !MRI->hasOneUse(MI.getOperand(3).getReg()) ||
+ !ensureDominates(MI.getOperand(2), *True))
+ return false;
+ True->getOperand(1).setReg(MI.getOperand(2).getReg());
+ }
+
MI.setDesc(TII->get(NewOpc));
MI.removeOperand(2); // False operand
MI.removeOperand(3); // Mask operand
diff --git a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-store-asm.ll b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-store-asm.ll
index 569ada7949b1b5..e57b6a22dd6eab 100644
--- a/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-store-asm.ll
+++ b/llvm/test/CodeGen/RISCV/rvv/fixed-vectors-strided-load-store-asm.ll
@@ -62,11 +62,12 @@ define void @gather_masked(ptr noalias nocapture %A, ptr noalias nocapture reado
; CHECK-NEXT: li a4, 5
; CHECK-NEXT: .LBB1_1: # %vector.body
; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
-; CHECK-NEXT: vsetvli zero, a3, e8, m1, ta, ma
-; CHECK-NEXT: vlse8.v v8, (a1), a4, v0.t
-; CHECK-NEXT: vle8.v v9, (a0)
-; CHECK-NEXT: vadd.vv v8, v9, v8
-; CHECK-NEXT: vse8.v v8, (a0)
+; CHECK-NEXT: vmv1r.v v9, v8
+; CHECK-NEXT: vsetvli zero, a3, e8, m1, ta, mu
+; CHECK-NEXT: vlse8.v v9, (a1), a4, v0.t
+; CHECK-NEXT: vle8.v v10, (a0)
+; CHECK-NEXT: vadd.vv v9, v10, v9
+; CHECK-NEXT: vse8.v v9, (a0)
; CHECK-NEXT: addi a0, a0, 32
; CHECK-NEXT: addi a1, a1, 160
; CHECK-NEXT: bne a0, a2, .LBB1_1
@@ -343,11 +344,12 @@ define void @scatter_masked(ptr noalias nocapture %A, ptr noalias nocapture read
; CHECK-NEXT: li a4, 5
; CHECK-NEXT: .LBB7_1: # %vector.body
; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
-; CHECK-NEXT: vsetvli zero, a3, e8, m1, ta, ma
-; CHECK-NEXT: vle8.v v8, (a1)
-; CHECK-NEXT: vlse8.v v9, (a0), a4, v0.t
-; CHECK-NEXT: vadd.vv v8, v9, v8
-; CHECK-NEXT: vsse8.v v8, (a0), a4, v0.t
+; CHECK-NEXT: vsetvli zero, a3, e8, m1, ta, mu
+; CHECK-NEXT: vle8.v v9, (a1)
+; CHECK-NEXT: vmv1r.v v10, v8
+; CHECK-NEXT: vlse8.v v10, (a0), a4, v0.t
+; CHECK-NEXT: vadd.vv v9, v10, v9
+; CHECK-NEXT: vsse8.v v9, (a0), a4, v0.t
; CHECK-NEXT: addi a1, a1, 32
; CHECK-NEXT: addi a0, a0, 160
; CHECK-NEXT: bne a1, a2, .LBB7_1
diff --git a/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-to-vmv.mir b/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-to-vmv.mir
index 875d4229bbc6e1..2383842bc49d97 100644
--- a/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-to-vmv.mir
+++ b/llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-to-vmv.mir
@@ -138,3 +138,24 @@ body: |
%true:vrnov0 = PseudoVADD_VV_M1_MASK %false, $noreg, $noreg, $v0, 4, 4 /* e16 */, 0 /* tu, mu */
$v0 = COPY %mask
%x:vrnov0 = PseudoVMERGE_VVM_M1 %pt, %false, %true, $v0, 8, 5 /* e32 */
+...
+---
+name: same_mask_undef_truepassthru
+body: |
+ bb.0:
+ liveins: $v8, $v0
+ ; CHECK-LABEL: name: same_mask_undef_truepassthru
+ ; CHECK: liveins: $v8, $v0
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: %false:vrnov0 = COPY $v8
+ ; CHECK-NEXT: %mask:vr = COPY $v0
+ ; CHECK-NEXT: $v0 = COPY %mask
+ ; CHECK-NEXT: %true:vrnov0 = PseudoVADD_VV_M1_MASK %false, $noreg, $noreg, $v0, 4, 5 /* e32 */, 1 /* ta, mu */
+ ; CHECK-NEXT: $v0 = COPY %mask
+ %false:vrnov0 = COPY $v8
+ %mask:vr = COPY $v0
+ $v0 = COPY %mask
+ %true:vrnov0 = PseudoVADD_VV_M1_MASK $noreg, $noreg, $noreg, $v0, 4, 5 /* e32 */, 0 /* tu, mu */
+ $v0 = COPY %mask
+ %x:vrnov0 = PseudoVMERGE_VVM_M1 $noreg, %false, %true, $v0, 4, 5 /* e32 */
+...
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/5207 Here is the relevant piece of the build log for the reference |
lukel97
added a commit
that referenced
this pull request
Sep 10, 2024
In #107827 we now set true's passthru to the false operand if it was undef. We need to remember to also constrain the regclass in case true is a masked pseudo which needs its passthrus to be in VR[M*]NoV0
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.
This fixes the issue raised in #106108 (comment)
True's passthru needs to be equivalent to vmerge's false, but we also allow true's passthru to be undef.
However if it's undef then we need to replace it with false, otherwise we end up discarding the false operand entirely.
The changes in fixed-vectors-strided-load-store-asm.ll undo the changes in #106108 where we introduced this miscompile.