[RISCV][VLOPT] Fix passthru check in getOperandInfo

[lukel97]

If a pseudo has a passthru, I believe the first source operand will have operand no 2, not 1.

I've added an MIR test which should show how we're reading the wrong SEW, but in order to be able to use -run-pass=riscv-vl-optimizer I needed to initialize the pass in RISCVTargetMachine.cpp.

[llvmbot]

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

Author: Luke Lau (lukel97)

Changes

If a pseudo has a passthru, I believe the first source operand will be the second operand, not the first.

I've added an MIR test which should show how we're reading the wrong SEW, but in order to be able to use -run-pass=riscv-vl-optimizer I needed to initialize the pass in RISCVTargetMachine.cpp.

---

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

3 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVTargetMachine.cpp (+1)
• (modified) llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp (+2-2)
• (added) llvm/test/CodeGen/RISCV/rvv/vl-opt.mir (+18)

diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index c48470ab707f10..089dc6c529193d 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -128,6 +128,7 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   initializeRISCVPreRAExpandPseudoPass(*PR);
   initializeRISCVExpandPseudoPass(*PR);
   initializeRISCVVectorPeepholePass(*PR);
+  initializeRISCVVLOptimizerPass(*PR);
   initializeRISCVInsertVSETVLIPass(*PR);
   initializeRISCVInsertReadWriteCSRPass(*PR);
   initializeRISCVInsertWriteVXRMPass(*PR);
diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
index eb1f4df4ff7264..d4d66c4e1cec58 100644
--- a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
@@ -431,7 +431,7 @@ static OperandInfo getOperandInfo(const MachineInstr &MI,
   case RISCV::VWMACCSU_VV:
   case RISCV::VWMACCSU_VX:
   case RISCV::VWMACCUS_VX: {
-    bool IsOp1 = HasPassthru ? MO.getOperandNo() == 1 : MO.getOperandNo() == 2;
+    bool IsOp1 = HasPassthru ? MO.getOperandNo() == 2 : MO.getOperandNo() == 1;
     bool TwoTimes = IsMODef || IsOp1;
     unsigned Log2EEW = TwoTimes ? MILog2SEW + 1 : MILog2SEW;
     RISCVII::VLMUL EMUL =
@@ -467,7 +467,7 @@ static OperandInfo getOperandInfo(const MachineInstr &MI,
   case RISCV::VNCLIP_WI:
   case RISCV::VNCLIP_WV:
   case RISCV::VNCLIP_WX: {
-    bool IsOp1 = HasPassthru ? MO.getOperandNo() == 1 : MO.getOperandNo() == 2;
+    bool IsOp1 = HasPassthru ? MO.getOperandNo() == 2 : MO.getOperandNo() == 1;
     bool TwoTimes = IsOp1;
     unsigned Log2EEW = TwoTimes ? MILog2SEW + 1 : MILog2SEW;
     RISCVII::VLMUL EMUL =
diff --git a/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
new file mode 100644
index 00000000000000..59a472c73a4624
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
@@ -0,0 +1,18 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc %s -o - -mtriple=riscv64 -mattr=+v -run-pass=riscv-vl-optimizer -verify-machineinstrs | FileCheck %s
+
+---
+name: vnsrl_wv_user
+body: |
+  bb.0:
+    liveins: $x1
+    ; CHECK-LABEL: name: vnsrl_wv_user
+    ; CHECK: liveins: $x1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %vl:gprnox0 = COPY $x1
+    ; CHECK-NEXT: %x:vr = PseudoVADD_VV_MF4 $noreg, $noreg, $noreg, -1, 4 /* e16 */, 0 /* tu, mu */
+    ; CHECK-NEXT: %y:vr = PseudoVNSRL_WV_MF4 $noreg, %x, $noreg, %vl, 4 /* e16 */, 0 /* tu, mu */
+    %vl:gprnox0 = COPY $x1
+    %x:vr = PseudoVADD_VV_MF4 $noreg, $noreg, $noreg, -1, 4 /* e16 */, 0 /* tu, mu */
+    %y:vr = PseudoVNSRL_WV_MF4 $noreg, %x, $noreg, %vl, 4 /* e16 */, 0 /* tu, mu */
+...

[michaelmaitland]

Do we need to fix in isVectorOpUsedAsScalarOp too?

[lukel97]

Do we need to fix in isVectorOpUsedAsScalarOp too?

I think isVectorOpUsedAsScalarOp is actually correct currently, but because HasPassthru is always true. I don't think it's correct if HasPassthru is false but reductions always have a passthru, so it's never hit. I think we can remove the check in a follow up NFC patch

  case RISCV::VFWREDUSUM_VS: {
    bool HasPassthru = RISCVII::isFirstDefTiedToFirstUse(MI->getDesc());
    return HasPassthru ? MO.getOperandNo() == 2 : MO.getOperandNo() == 3;

[michaelmaitland]

LGTM

[lukel97]

Do we need to fix in isVectorOpUsedAsScalarOp too?

I think isVectorOpUsedAsScalarOp is actually correct currently, but because HasPassthru is always true.

Actually I think I was wrong, I think the scalar ops should be 1 (vd) and 3 (vs1). But this can be fixed in a separate PR