[RISCV] Don't relax policy to ta when vmerge's VL shrinks during folding

[luke]

Summary

When folding a vmerge into its operands, if the resulting VL is smaller than
what the vmerge had originally then what was previously in its body then gets
moved to the tail. In that case, we can't relax the tail policy to agnostic
when the merge operand is undefined, since we need to preserve these elements
past the new VL.

Fixes https://github.com/llvm/llvm-project/issues/64754

[luke]

Rebase

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3512

If instead, we were to change this line to:

uint64_t Policy = isImplicitDef(False)  ? RISCVII::TAIL_AGNOSTIC : /*TUMU*/ 0;

What would happen? I'm worried there's some other case we haven't caught where Merge was implicit_def, but the resulting operation has a non-implicit_def result. I think *all* such cases needs to be TU.

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3512

Answer: A lot of regressions. I tried this, and it definitely doesn't seem to work.

I'm still a bit worried there's another case here. I don't have anything specific though.

[luke]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3512

The new tail of the Result node is every element from min(vmerge.vl, true.vl) onwards.

Therefore if we want to set a tail agnostic policy on it, we need to know that every element of vmerge from min(vmerge.vl, true.vl) onwards would have been implicit-def anyway.

The vmerge pseudo gets its tail from Merge, so if the Merge at that element was implicit-def, then that tail element will be implicit-def.

implicit-def(Merge[i])
->
implicit-def(vmerge[i]), vmerge.vl <= i < vlmax

We can rewrite this to

implicit-def(Merge[i]) ∧ vmerge.vl == min(vmerge.vl, true.vl)
->
implicit-def(vmerge[i]), min(vmerge.vl, true.vl) <= i < vlmax

So as long as we check isImplicitDef(Merge) and VL == OldVL, then we can infer the result

implicit-def(vmerge[i]), min(vmerge.vl, true.vl) <= i < vlmax

i.e. every element past min(vmerge.vl, true.vl) in vmerge would have been implicit-def.

Btw I tried substituting Merge for False as well before posting the patch and got the same result.

implicit(False) -> implicit(Merge) so as long as we also check the VL stays the same, it's a correct transformation, just not optimal.

[craig.topper]

LGTM, but maybe wait until @reames approves too

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3512

80 columns

[reames]

LGTM. Sorry, should have done that Friday.

[reames]

In D158161#4604128, @reames wrote:

LGTM. Sorry, should have done that Friday.

I sat down and did the case analysis for all 9 combinations of policy states. We disallow certain combinations via early returns, and I believe that with this patch, all the other cases are covered correctly.

I think this might be a case where generalizing to handle the remaining cases actually simplifies and makes the code more obvious, but that's definitely a future patch!

[luke]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3512

Fixed in commit

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

auto GetMinVL = [](SDValue LHS, SDValue RHS) {
     auto *CRHS = dyn_cast<ConstantSDNode>(RHS);
     if (!CLHS || !CRHS)
       return SDValue();
     return CLHS->getZExtValue() <= CRHS->getZExtValue() ? LHS : RHS;
   };
 
   // Because N and True must have the same merge operand (or True's operand is
   // implicit_def), the "effective" body is the minimum of their VLs.
+  SDValue OrigVL = VL;
   VL = GetMinVL(TrueVL, VL);
   if (!VL)
     return false;
 
   // If we end up changing the VL or mask of True, then we need to make sure it
   // doesn't raise any observable fp exceptions, since changing the active
   // elements will affect how fflags is set.
   if (TrueVL != VL || !IsMasked)
#ifndef NDEBUG
   const MCInstrDesc &MaskedMCID = TII->get(MaskedOpc);
   assert(RISCVII::hasVecPolicyOp(MaskedMCID.TSFlags) &&
          "Expected instructions with mask have policy operand.");
   assert(MaskedMCID.getOperandConstraint(MaskedMCID.getNumDefs(),
                                          MCOI::TIED_TO) == 0 &&
          "Expected instructions with mask have a tied dest.");
 #endif
 
-  uint64_t Policy = isImplicitDef(Merge) ? RISCVII::TAIL_AGNOSTIC : /*TUMU*/ 0;
+  // Use a tumu policy, relaxing it to tail agnostic provided that the merge
+  // operand is undefined.
+  //
+  // However, if the VL became smaller than what the vmerge had originally, then
+  // elements past VL that were previously in the vmerge's body will have moved
+  // to the tail. In that case we always need to use tail undisturbed to
+  // preserve them.
+  bool MergeVLShrunk = VL != OrigVL;
+  uint64_t Policy = (isImplicitDef(Merge) && !MergeVLShrunk) ? RISCVII::TAIL_AGNOSTIC : /*TUMU*/ 0;
   SDValue PolicyOp =
     CurDAG->getTargetConstant(Policy, DL, Subtarget->getXLenVT());
 
 
   SmallVector<SDValue, 8> Ops;
   Ops.push_back(False);
 
   const bool HasRoundingMode = RISCVII::hasRoundModeOp(TrueTSFlags);

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vselect.ll

 ; RV32-NEXT:    slli a1, a2, 27
 ; RV32-NEXT:    srli a1, a1, 31
 ; RV32-NEXT:    vslide1down.vx v10, v10, a1
 ; RV32-NEXT:    srli a2, a2, 5
 ; RV32-NEXT:    vslide1down.vx v10, v10, a2
 ; RV32-NEXT:    vslidedown.vi v10, v10, 2
 ; RV32-NEXT:    vand.vi v10, v10, 1
 ; RV32-NEXT:    vmsne.vi v0, v10, 0
-; RV32-NEXT:    vsetivli zero, 6, e32, m2, ta, mu
+; RV32-NEXT:    vsetivli zero, 6, e32, m2, tu, mu
 ; RV32-NEXT:    vle32.v v8, (a0), v0.t
 ; RV32-NEXT:    vse32.v v8, (a3)
 ; RV32-NEXT:    ret
 ;
 ; RV64-LABEL: vselect_vv_v6i32:
 ; RV64:       # %bb.0:
 ; RV64-NEXT:    vsetivli zero, 6, e32, m2, ta, ma
 ; RV64-NEXT:    lbu a2, 0(a2)

 ; RV64-NEXT:    slli a1, a2, 59
 ; RV64-NEXT:    srli a1, a1, 63
 ; RV64-NEXT:    vslide1down.vx v10, v10, a1
 ; RV64-NEXT:    srli a2, a2, 5
 ; RV64-NEXT:    vslide1down.vx v10, v10, a2
 ; RV64-NEXT:    vslidedown.vi v10, v10, 2
 ; RV64-NEXT:    vand.vi v10, v10, 1
 ; RV64-NEXT:    vmsne.vi v0, v10, 0
-; RV64-NEXT:    vsetivli zero, 6, e32, m2, ta, mu
+; RV64-NEXT:    vsetivli zero, 6, e32, m2, tu, mu
 ; RV64-NEXT:    vle32.v v8, (a0), v0.t
 ; RV64-NEXT:    vse32.v v8, (a3)
 ; RV64-NEXT:    ret
   %va = load <6 x i32>, ptr %a
   %vb = load <6 x i32>, ptr %b
   %vcc = load <6 x i1>, ptr %cc
   %vsel = select <6 x i1> %vcc, <6 x i32> %va, <6 x i32> %vb
   store <6 x i32> %vsel, ptr %z

 ; RV32-NEXT:    slli a1, a2, 27
 ; RV32-NEXT:    srli a1, a1, 31
 ; RV32-NEXT:    vslide1down.vx v10, v10, a1
 ; RV32-NEXT:    srli a2, a2, 5
 ; RV32-NEXT:    vslide1down.vx v10, v10, a2
 ; RV32-NEXT:    vslidedown.vi v10, v10, 2
 ; RV32-NEXT:    vand.vi v10, v10, 1
 ; RV32-NEXT:    vmsne.vi v0, v10, 0
-; RV32-NEXT:    vsetivli zero, 6, e32, m2, ta, mu
+; RV32-NEXT:    vsetivli zero, 6, e32, m2, tu, mu
 ; RV32-NEXT:    vle32.v v8, (a0), v0.t
 ; RV32-NEXT:    vse32.v v8, (a3)
 ; RV32-NEXT:    ret
 ;
 ; RV64-LABEL: vselect_vv_v6f32:
 ; RV64:       # %bb.0:
 ; RV64-NEXT:    vsetivli zero, 6, e32, m2, ta, ma
 ; RV64-NEXT:    lbu a2, 0(a2)

 ; RV64-NEXT:    slli a1, a2, 59
 ; RV64-NEXT:    srli a1, a1, 63
 ; RV64-NEXT:    vslide1down.vx v10, v10, a1
 ; RV64-NEXT:    srli a2, a2, 5
 ; RV64-NEXT:    vslide1down.vx v10, v10, a2
 ; RV64-NEXT:    vslidedown.vi v10, v10, 2
 ; RV64-NEXT:    vand.vi v10, v10, 1
 ; RV64-NEXT:    vmsne.vi v0, v10, 0
-; RV64-NEXT:    vsetivli zero, 6, e32, m2, ta, mu
+; RV64-NEXT:    vsetivli zero, 6, e32, m2, tu, mu
 ; RV64-NEXT:    vle32.v v8, (a0), v0.t
 ; RV64-NEXT:    vse32.v v8, (a3)
 ; RV64-NEXT:    ret
   %va = load <6 x float>, ptr %a
   %vb = load <6 x float>, ptr %b
   %vcc = load <6 x i1>, ptr %cc
   %vsel = select <6 x i1> %vcc, <6 x float> %va, <6 x float> %vb
   store <6 x float> %vsel, ptr %z

llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll

 ; CHECK-NEXT:    vsetivli zero, 2, e32, m1, tu, mu
 ; CHECK-NEXT:    vadd.vv v8, v9, v10, v0.t
 ; CHECK-NEXT:    ret
   %a = call <vscale x 2 x i32> @llvm.riscv.vadd.nxv2i32.nxv2i32(<vscale x 2 x i32> poison, <vscale x 2 x i32> %x, <vscale x 2 x i32> %y, i64 2)
   %b = call <vscale x 2 x i32> @llvm.riscv.vmerge.nxv2i32.nxv2i32(<vscale x 2 x i32> %passthru, <vscale x 2 x i32> %passthru, <vscale x 2 x i32> %a, <vscale x 2 x i1> %m, i64 3)
   ret <vscale x 2 x i32> %b
 }
 
-; FIXME: The vadd's new policy should be tail undisturbed since the false op of
-; the vmerge moves from the the body to the tail, and we need to preserve it.
+; The vadd's new policy should be tail undisturbed since the false op of the
+; vmerge moves from the the body to the tail, and we need to preserve it.
 define <vscale x 2 x i32> @vmerge_larger_vl_false_becomes_tail(<vscale x 2 x i32> %false, <vscale x 2 x i32> %x, <vscale x 2 x i32> %y, <vscale x 2 x i1> %m) {
 ; CHECK-LABEL: vmerge_larger_vl_false_becomes_tail:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    vsetivli zero, 2, e32, m1, ta, mu
+; CHECK-NEXT:    vsetivli zero, 2, e32, m1, tu, mu
 ; CHECK-NEXT:    vadd.vv v8, v9, v10, v0.t
 ; CHECK-NEXT:    ret
   %a = call <vscale x 2 x i32> @llvm.riscv.vadd.nxv2i32.nxv2i32(<vscale x 2 x i32> poison, <vscale x 2 x i32> %x, <vscale x 2 x i32> %y, i64 2)
   %b = call <vscale x 2 x i32> @llvm.riscv.vmerge.nxv2i32.nxv2i32(<vscale x 2 x i32> poison, <vscale x 2 x i32> %false, <vscale x 2 x i32> %a, <vscale x 2 x i1> %m, i64 3)
   ret <vscale x 2 x i32> %b
 }