From a35e91e1a5f967f847d847638ba8758d8b0eb337 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Tue, 28 Jan 2025 19:34:28 +0800 Subject: [PATCH 1/3] [RISCV][VLOPT] Fix assertion failure across blocks 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. --- llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp | 10 ----- llvm/test/CodeGen/RISCV/rvv/vl-opt.mir | 46 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp index e8c01e57038bf..9bd3895489da1 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 027eb8ca3c17f..7a28eaaaa5d8b 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 From 4ca7c87861882d44175689fd52863db87d0a2974 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Tue, 28 Jan 2025 23:52:16 +0800 Subject: [PATCH 2/3] Keep VL = 1 check, check isCandidate after popping instead --- llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp index 9bd3895489da1..cb0c1b82b734d 100644 --- a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp +++ b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp @@ -1143,6 +1143,16 @@ 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; @@ -1335,8 +1345,6 @@ bool RISCVVLOptimizer::runOnMachineFunction(MachineFunction &MF) { continue; MachineInstr *DefMI = MRI->getVRegDef(Op.getReg()); - if (!isCandidate(*DefMI)) - continue; if (IgnoreSameBlock && DefMI->getParent() == MI.getParent()) continue; @@ -1368,7 +1376,8 @@ bool RISCVVLOptimizer::runOnMachineFunction(MachineFunction &MF) { while (!Worklist.empty()) { assert(MadeChange); MachineInstr &MI = *Worklist.pop_back_val(); - assert(isCandidate(MI)); + if (!isCandidate(MI)) + continue; if (!tryReduceVL(MI)) continue; PushOperands(MI, /*IgnoreSameBlock*/ false); From e0781aa8dc3029613f8771837ebde897cc6fd180 Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Wed, 29 Jan 2025 01:05:17 +0800 Subject: [PATCH 3/3] Move VL=1 check to tryReduceVL --- llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp | 28 ++++++++++------------ 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp index cb0c1b82b734d..d9e62449490de 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; @@ -1285,6 +1275,16 @@ std::optional RISCVVLOptimizer::checkUsers(MachineInstr &MI) { bool RISCVVLOptimizer::tryReduceVL(MachineInstr &MI) { LLVM_DEBUG(dbgs() << "Trying to reduce VL for " << MI << "\n"); + unsigned VLOpNum = RISCVII::getVLOpNum(MI.getDesc()); + 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() << " Abort due to VL == 1, no point in reducing.\n"); + return false; + } + auto CommonVL = checkUsers(MI); if (!CommonVL) return false; @@ -1292,9 +1292,6 @@ bool RISCVVLOptimizer::tryReduceVL(MachineInstr &MI) { assert((CommonVL->isImm() || CommonVL->getReg().isVirtual()) && "Expected VL to be an Imm or virtual Reg"); - unsigned VLOpNum = RISCVII::getVLOpNum(MI.getDesc()); - MachineOperand &VLOp = MI.getOperand(VLOpNum); - if (!RISCV::isVLKnownLE(*CommonVL, VLOp)) { LLVM_DEBUG(dbgs() << " Abort due to CommonVL not <= VLOp.\n"); return false; @@ -1345,6 +1342,8 @@ bool RISCVVLOptimizer::runOnMachineFunction(MachineFunction &MF) { continue; MachineInstr *DefMI = MRI->getVRegDef(Op.getReg()); + if (!isCandidate(*DefMI)) + continue; if (IgnoreSameBlock && DefMI->getParent() == MI.getParent()) continue; @@ -1376,8 +1375,7 @@ bool RISCVVLOptimizer::runOnMachineFunction(MachineFunction &MF) { while (!Worklist.empty()) { assert(MadeChange); MachineInstr &MI = *Worklist.pop_back_val(); - if (!isCandidate(MI)) - continue; + assert(isCandidate(MI)); if (!tryReduceVL(MI)) continue; PushOperands(MI, /*IgnoreSameBlock*/ false);