[RISCV][VLOPT] Fix operand check in isVectorOpUsedAsScalarOp

[lukel97]

A reduction instruction always has a passthru operand, so the scalar operand should always be vs1 which is at index 3.

Even though the destination operand is also scalar, I think the passthru will need to preserve all elements so I haven't included it.

[llvmbot]

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

Author: Luke Lau (lukel97)

Changes

A reduction instruction always has a passthru operand, so the scalar operand should always be vs1 which is at index 3.

Even though the destination operand is also scalar, I think the passthru will need to preserve all elements so I haven't included it.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp (+2-4)
• (modified) llvm/test/CodeGen/RISCV/rvv/vl-opt.mir (+17)

diff --git a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
index 1a9084f8b6cb2b..8a4dd70d961f8b 100644
--- a/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
+++ b/llvm/lib/Target/RISCV/RISCVVLOptimizer.cpp
@@ -611,10 +611,8 @@ static bool isVectorOpUsedAsScalarOp(MachineOperand &MO) {
   case RISCV::VFREDOSUM_VS:
   case RISCV::VFREDUSUM_VS:
   case RISCV::VFWREDOSUM_VS:
-  case RISCV::VFWREDUSUM_VS: {
-    bool HasPassthru = RISCVII::isFirstDefTiedToFirstUse(MI->getDesc());
-    return HasPassthru ? MO.getOperandNo() == 2 : MO.getOperandNo() == 3;
-  }
+  case RISCV::VFWREDUSUM_VS:
+    return MO.getOperandNo() == 3;
   default:
     return false;
   }
diff --git a/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
index 59a472c73a4624..010e3ca642269b 100644
--- a/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
+++ b/llvm/test/CodeGen/RISCV/rvv/vl-opt.mir
@@ -16,3 +16,20 @@ body: |
     %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 */
 ...
+---
+name: vredsum_vv_user
+body: |
+  bb.0:
+    liveins: $x1
+    ; CHECK-LABEL: name: vredsum_vv_user
+    ; CHECK: liveins: $x1
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: %vl:gprnox0 = COPY $x1
+    ; CHECK-NEXT: %x:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, -1, 5 /* e32 */, 0 /* tu, mu */
+    ; CHECK-NEXT: %y:vr = PseudoVREDSUM_VS_M1_E64 $noreg, %x, $noreg, -1, 6 /* e64 */, 0 /* tu, mu */
+    ; CHECK-NEXT: %z:vr = PseudoVADD_VV_M1 $noreg, %x, $noreg, %vl, 5 /* e32 */, 0 /* tu, mu */
+    %vl:gprnox0 = COPY $x1
+    %x:vr = PseudoVADD_VV_M1 $noreg, $noreg, $noreg, -1, 5 /* e32 */, 0 /* tu, mu */
+    %y:vr = PseudoVREDSUM_VS_M1_E64 $noreg, %x, $noreg, -1, 6 /* e64 */, 0 /* tu, mu */
+    %z:vr = PseudoVADD_VV_M1 $noreg, %x, $noreg, %vl, 5 /* e32 */, 0 /* tu, mu */
+...

[michaelmaitland]

LGTM

[michaelmaitland]

I think the passthru will need to preserve all elements so I haven't included it.

I am seeing this from the spec:

The other elements in the destination vector register ( 0 < index < VLEN/SEW) are considered the tail and are managed with the current tail agnostic/undisturbed policy.

IIUC, I think this is saying we only need to preserve elements of the passthru depending on tail policy. I think we could maybe treat the destination as a scalar operand if the policy is TA?

[lukel97]

IIUC, I think this is saying we only need to preserve elements of the passthru depending on tail policy. I think we could maybe treat the destination as a scalar operand if the policy is TA?

If I'm reading this right it shouldn't matter if we mark the destination operand as scalar or not because isVectorOpUsedAsScalarOp is only called on users.

The passthru operand might be checked, but if the policy is TA then I think we still need to preserve it. Because tail agnostic still somewhat defined (it has to be either the same as the passthru or all ones), which is separate from "tail undefined" when we have an undef passthru.

[michaelmaitland]

IIUC, I think this is saying we only need to preserve elements of the passthru depending on tail policy. I think we could maybe treat the destination as a scalar operand if the policy is TA?

If I'm reading this right it shouldn't matter if we mark the destination operand as scalar or not because isVectorOpUsedAsScalarOp is only called on users.

The passthru operand might be checked, but if the policy is TA then I think we still need to preserve it. Because tail agnostic still somewhat defined (it has to be either the same as the passthru or all ones), which is separate from "tail undefined" when we have an undef passthru.

Okay, LGTM.