[RISCV][RVV] Select unmasked TU RVV pseudos in a DAG post-process

[arcbbb]

Summary

Following D118810 that reduced the size of ISel table,
this patch optimizes allone-masked RVV pseudos with TU policy and swap them
out to their unmasked TU pseudos.

Since the UNDEF merge operand is not preserved, we turn it into TA pseudo regardless of the policy operand.

[rogfer01]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2282

Sorry, I fail to follow the logic here. I'm sure I'm missing something very obvious.

My understanding is that the original code refrained itself from doing this optimization because it picks a pseudo that does not have a merge operand, so a tail policy that is not agnostic (i.e. tail undisturbed) is not eligible.

Your change allows picking a tail undisturbed pseudo when the tail policy is undisturbed (i.e. when the tail policy is not agnostic), why is then having an undef as a merge operand a problem?

In fact I think this is what @frasercrmck mentioned here https://reviews.llvm.org/D118810#3293768

One option, also mentioned by @frasercrmck in https://reviews.llvm.org/D118810#3296415, is to convert this specific case into a tail agnostic case, rather than giving up as your change does.

Does this make sense?

[arcbbb]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2282

Yes you are right, I think I am hesitant to make a decision whether to replace it with unmasked TA or unmasked TU.
Both seems reasonable to me, and besides I don't have a test case for that yet, maybe I should leave a FIXME here before we know how to handle it?

[arcbbb]

Updates:

1. Addresses @rogfer01's comment. Transforms TU+IMPLICIT_DEF to TA. This can be changed later if we have alternative idea.
2. Adds TU+IMPLICIT_DEF test cases by

    sed -i '/; CHECK.*/d' $file
    sed -i -e '/define.*@intrinsic_vf.\?cvt_mask/{
           h;
           s/define <vscale.*> @intrinsic_\(vf.\?cvt\)_mask\(.*\)(\(<vscale.*>\) %0, \(<vscale.*>\) %1, <vscale x \(.*\) x i1> %2, iXLen %3) \(.*\)/\
define \3 @intrinsic_allone_\1_mask\2(\4 %1, iXLen %3) \6\
entry:\
  %allone = call <vscale x \5 x i1> @llvm.riscv.vmset.nxv\5i1(\
    iXLen %3);/;
           x;
           n;n;N;N; N;N;N;N;N;
           H;
           x;
           s/%0/undef/g
           s/%1/%0/g
           s/%2/%allone/g
           s/%3/%1/g
           s/iXLen 1/iXLen 0/g
           x;
           G;}

[frasercrmck]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

If it's TAIL_UNDISTURBED and the merge operand isn't undef, shouldn't we be bailing?

2311

Id isn't a good name: sounds to me like "identifier"

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

4773 ↗

(On Diff #416207)

Feels like this change should go into D120449?

[khchen]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

I think we had not discuss what's proper way to handle conflict values between merge and policy operand.

1. merge operand is not a undef value but policy is agnostic. (ex. tama)
2. merge operand is a undef value but policy is undisturbed. (ex. tamu/tuma/tumu)

In fact, I didn't have strong opinion here. so in https://reviews.llvm.org/D120226 patch, I changed VSETVLI inserter to update vtype according to the policy value directly rather than checking merge operand is undef or not to do something (ex. report an error, or auto fixup), because I think users have explicitly specific the policy value, we need to respect it.

auto fixup means backend could update the policy value for 1. with tumu and 2. with tama.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

From the register allocation standpoint, an undef operand gives the register allocator permission to pick whatever register is convenient. So it definitely won't preserve anything in a way a user could rely on. So in that case the policy is only going to affect the vsetlvli.

[arcbbb]

Updates:

1. Renames Id to I, and IsTU to IsTA
2. Makes this patch independent of D120449.

[arcbbb]

Fixed typo in the test filename.

[arcbbb]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

I discover that the inserted vsetvli uses TA for TU pseudos with IMPLICIT_DEF tie operand, so I follow the logic in computeInfoForInstr at RISCVInsertVSETVLI.cpp to transform it into TA pseudo.
Does this make sense?

[arcbbb]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

@frasercrmck Does this make sense to you?

[arcbbb]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

429

After D123217 there are cases where _TU pseudo doesn't exist.
I have to fix this logic.

[arcbbb]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

@frasercrmck Sorry for the noise. I didn't notice the enum was changed.

[frasercrmck]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2281

Sorry, I got a bit lost by the conversation and then it dropped off my radar. I think it's resolved though? Converting TU + IMPLICIT_DEF to TA makes sense to me.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

429

I'm sorry about that. Can we make it optional somehow?

llvm/test/CodeGen/RISCV/rvv/allone-masked-to-unmasked.ll

1

Could these tests be pre-commited?

[arcbbb]

Updates

1. Add a placeholder for pseudos that don't have _TU variants.

[arcbbb]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

429

It just happened sooner than I thought it would be.
Not sure if I have done it right, I create a pseudo as a placeholder.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2269–2270

"we're expecting" here doesn't make sense does it? It's more like "but we might have a Glue operand last".

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

429

Can you use !if instead of !cond? Then you don't need the true : part

429

Instead of having a Dummy Pseudo, could you use the MaskedPseudo value. Then in RISCVISelDAGToDAG.cpp detect that the return value was the same opcode you started with?

[arcbbb]

Address Craig's comments.

[arcbbb]

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

429

It looks better now. Thanks!

[frasercrmck]

I'm glad we don't need to add dummy pseudos for this to work :)

This is now looking good, thank you: just to check - there's no way to dynamically check whether the TU pseudo exists in TableGen? That would remove the need for HasTU which I'd find a bit cleaner.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2284

This comment should probably be inverted. Something like "if the merge operand isn't IMPLICIT_DEF then we can't use TA"?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

427

Can get rid of this now it's unused?

[arcbbb]

Address @frasercrmck 's comments

[arcbbb]

In D121881#3447794, @frasercrmck wrote:

I'm glad we don't need to add dummy pseudos for this to work :)

This is now looking good, thank you: just to check - there's no way to dynamically check whether the TU pseudo exists in TableGen? That would remove the need for HasTU which I'd find a bit cleaner.

I read through the TableGen document several times for this, but it is a pity that I didn't discover any operator that can help.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2291

Could shorten the checks in the assert by doing

uint64_t TSFlags = TII->get(I->UnmaskedPseudo).TSFlags;

2301

Same here

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.h

194

Probably should be UnmaskedTUPseudo, since TU is an abbreviation and not a word.

[arcbbb]

Address Craig's comments.

[craig.topper]

LGTM

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.h

struct VLX_VSXPseudo {
   uint16_t LMUL : 3;
   uint16_t IndexLMUL : 3;
   uint16_t Pseudo;
 };
 
 struct RISCVMaskedPseudoInfo {
   uint16_t MaskedPseudo;
   uint16_t UnmaskedPseudo;
+  uint16_t UnmaskedTUPseudo;
   uint8_t MaskOpIdx;
 };
 
 #define GET_RISCVVSSEGTable_DECL
 #define GET_RISCVVLSEGTable_DECL
 #define GET_RISCVVLXSEGTable_DECL
 #define GET_RISCVVSXSEGTable_DECL
 #define GET_RISCVVLETable_DECL

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

bool RISCVDAGToDAGISel::doPeepholeMaskedRVV(SDNode *N) {
 
   // Retrieve the tail policy operand index, if any.
   Optional<unsigned> TailPolicyOpIdx;
   const RISCVInstrInfo *TII = static_cast<const RISCVInstrInfo *>(
       CurDAG->getSubtarget().getInstrInfo());
 
   const MCInstrDesc &MaskedMCID = TII->get(N->getMachineOpcode());
 
+  bool IsTA = true;
   if (RISCVII::hasVecPolicyOp(MaskedMCID.TSFlags)) {
-    // The last operand of the pseudo is the policy op, but we're expecting a
-    // Glue operand last. We may also have a chain.
+    // The last operand of the pseudo is the policy op, but we might have a
+    // Glue operand last. We might also have a chain.
     TailPolicyOpIdx = N->getNumOperands() - 1;
     if (N->getOperand(*TailPolicyOpIdx).getValueType() == MVT::Glue)
       (*TailPolicyOpIdx)--;
     if (N->getOperand(*TailPolicyOpIdx).getValueType() == MVT::Other)
       (*TailPolicyOpIdx)--;
 
-    // If the policy isn't TAIL_AGNOSTIC we can't perform this optimization.
-    if (N->getConstantOperandVal(*TailPolicyOpIdx) != RISCVII::TAIL_AGNOSTIC)
+    if (!(N->getConstantOperandVal(*TailPolicyOpIdx) &
+          RISCVII::TAIL_AGNOSTIC)) {
+      // Keep the true-masked instruction when there is no unmasked TU
+      // instruction
+      if (I->UnmaskedTUPseudo == I->MaskedPseudo && !N->getOperand(0).isUndef())
         return false;
+      // We can't use TA if the tie-operand is not IMPLICIT_DEF
+      if (!N->getOperand(0).isUndef())
+        IsTA = false;
+    }
   }
 
-  const MCInstrDesc &UnmaskedMCID = TII->get(I->UnmaskedPseudo);
+  if (IsTA) {
+    uint64_t TSFlags = TII->get(I->UnmaskedPseudo).TSFlags;
 
     // Check that we're dropping the merge operand, the mask operand, and any
     // policy operand when we transform to this unmasked pseudo.
-  assert(!RISCVII::hasMergeOp(UnmaskedMCID.TSFlags) &&
-         RISCVII::hasDummyMaskOp(UnmaskedMCID.TSFlags) &&
-         !RISCVII::hasVecPolicyOp(UnmaskedMCID.TSFlags) &&
+    assert(!RISCVII::hasMergeOp(TSFlags) && RISCVII::hasDummyMaskOp(TSFlags) &&
+           !RISCVII::hasVecPolicyOp(TSFlags) &&
+           "Unexpected pseudo to transform to");
+    (void)TSFlags;
+  } else {
+    uint64_t TSFlags = TII->get(I->UnmaskedTUPseudo).TSFlags;
+
+    // Check that we're dropping the mask operand, and any policy operand
+    // when we transform to this unmasked tu pseudo.
+    assert(RISCVII::hasMergeOp(TSFlags) && RISCVII::hasDummyMaskOp(TSFlags) &&
+           !RISCVII::hasVecPolicyOp(TSFlags) &&
            "Unexpected pseudo to transform to");
-  (void)UnmaskedMCID;
+    (void)TSFlags;
+  }
 
+  unsigned Opc = IsTA ? I->UnmaskedPseudo : I->UnmaskedTUPseudo;
   SmallVector<SDValue, 8> Ops;
-  // Skip the merge operand at index 0.
-  for (unsigned I = 1, E = N->getNumOperands(); I != E; I++) {
+  // Skip the merge operand at index 0 if IsTA
+  for (unsigned I = IsTA, E = N->getNumOperands(); I != E; I++) {
     // Skip the mask, the policy, and the Glue.
     SDValue Op = N->getOperand(I);
     if (I == MaskOpIdx || I == TailPolicyOpIdx ||
         Op.getValueType() == MVT::Glue)
       continue;
     Ops.push_back(Op);
   }
 
   // Transitively apply any node glued to our new node.
   if (auto *TGlued = Glued->getGluedNode())
     Ops.push_back(SDValue(TGlued, TGlued->getNumValues() - 1));
 
-  SDNode *Result =
-      CurDAG->getMachineNode(I->UnmaskedPseudo, SDLoc(N), N->getVTList(), Ops);
+  SDNode *Result = CurDAG->getMachineNode(Opc, SDLoc(N), N->getVTList(), Ops);
   ReplaceUses(N, Result);
 
   return true;
 }
 
 // This pass converts a legalized DAG into a RISCV-specific DAG, ready
 // for instruction scheduling.
 FunctionPass *llvm::createRISCVISelDag(RISCVTargetMachine &TM) {
   return new RISCVDAGToDAGISel(TM);
 }

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td

 def RISCVVIntrinsicsTable : GenericTable {
   let FilterClass = "RISCVVIntrinsic";
   let CppTypeName = "RISCVVIntrinsicInfo";
   let Fields = ["IntrinsicID", "ScalarOperand", "VLOperand"];
   let PrimaryKey = ["IntrinsicID"];
   let PrimaryKeyName = "getRISCVVIntrinsicInfo";
 }
 
-class RISCVMaskedPseudo<bits<4> MaskIdx> {
+class RISCVMaskedPseudo<bits<4> MaskIdx, bit HasTU = true> {
   Pseudo MaskedPseudo = !cast<Pseudo>(NAME);
   Pseudo UnmaskedPseudo = !cast<Pseudo>(!subst("_MASK", "", NAME));
+  Pseudo UnmaskedTUPseudo = !if(HasTU, !cast<Pseudo>(!subst("_MASK", "", NAME # "_TU")), MaskedPseudo);
   bits<4> MaskOpIdx = MaskIdx;
 }
 
 def RISCVMaskedPseudosTable : GenericTable {
   let FilterClass = "RISCVMaskedPseudo";
   let CppTypeName = "RISCVMaskedPseudoInfo";
-  let Fields = ["MaskedPseudo", "UnmaskedPseudo", "MaskOpIdx"];
+  let Fields = ["MaskedPseudo", "UnmaskedPseudo", "UnmaskedTUPseudo", "MaskOpIdx"];
   let PrimaryKey = ["MaskedPseudo"];
   let PrimaryKeyName = "getMaskedPseudoInfo";
 }
 
 class RISCVVLE<bit M, bit TU, bit Str, bit F, bits<3> S, bits<3> L> {
   bits<1> Masked = M;
   bits<1> IsTU = TU;
   bits<1> Strided = Str;
multiclass VPseudoBinaryM<VReg RetClass,
                           LMULInfo MInfo,
                           string Constraint = ""> {
   let VLMul = MInfo.value in {
     def "_" # MInfo.MX : VPseudoBinaryNoMask<RetClass, Op1Class, Op2Class,
                                              Constraint>;
     let ForceTailAgnostic = true in
     def "_" # MInfo.MX # "_MASK" : VPseudoBinaryMOutMask<RetClass, Op1Class,
                                                          Op2Class, Constraint>,
-                                   RISCVMaskedPseudo</*MaskOpIdx*/ 3>;
+                                   RISCVMaskedPseudo</*MaskOpIdx*/ 3, /*HasTU*/ false>;
   }
 }
 
 multiclass VPseudoBinaryEmul<VReg RetClass,
                              VReg Op1Class,
                              DAGOperand Op2Class,
                              LMULInfo lmul,
                              LMULInfo emul,

llvm/test/CodeGen/RISCV/rvv/allone-masked-to-unmasked.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: sed 's/iXLen/i32/g' %s | llc -mtriple=riscv32 -mattr=+v -verify-machineinstrs | FileCheck %s
 ; RUN: sed 's/iXLen/i64/g' %s | llc -mtriple=riscv64 -mattr=+v -verify-machineinstrs | FileCheck %s
 
 declare <vscale x 1 x i1> @llvm.riscv.vmset.nxv1i1(iXLen);
 
 declare <vscale x 1 x i8> @llvm.riscv.vadd.mask.nxv1i8.nxv1i8(
   <vscale x 1 x i8>,
   <vscale x 1 x i8>,
%a = call <vscale x 1 x i8> @llvm.riscv.vadd.mask.nxv1i8.nxv1i8(
     <vscale x 1 x i8> %0,
     <vscale x 1 x i8> %1,
     <vscale x 1 x i1> %allone,
     iXLen %2, iXLen 1)
 
   ret <vscale x 1 x i8> %a
 }
 
-; FIXME: Use an unmasked TAIL_AGNOSTIC instruction if the tie operand is IMPLICIT_DEF
+; Use an unmasked TAIL_AGNOSTIC instruction if the tie operand is IMPLICIT_DEF
 define <vscale x 1 x i8> @test1(<vscale x 1 x i8> %0, <vscale x 1 x i8> %1, iXLen %2) nounwind {
 ; CHECK-LABEL: test1:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    vsetvli zero, a0, e8, mf8, ta, mu
-; CHECK-NEXT:    vmset.m v0
-; CHECK-NEXT:    vsetvli zero, zero, e8, mf8, tu, mu
-; CHECK-NEXT:    vadd.vv v8, v8, v9, v0.t
+; CHECK-NEXT:    vadd.vv v8, v8, v9
 ; CHECK-NEXT:    ret
 entry:
   %allone = call <vscale x 1 x i1> @llvm.riscv.vmset.nxv1i1(
     iXLen %2);
   %a = call <vscale x 1 x i8> @llvm.riscv.vadd.mask.nxv1i8.nxv1i8(
     <vscale x 1 x i8> undef,
     <vscale x 1 x i8> %0,
     <vscale x 1 x i8> %1,
     <vscale x 1 x i1> %allone,
     iXLen %2, iXLen 0)
 
   ret <vscale x 1 x i8> %a
 }
 
-; FIXME: Use an unmasked TU instruction because of the policy operand
+; Use an unmasked TU instruction because of the policy operand
 define <vscale x 1 x i8> @test2(<vscale x 1 x i8> %0, <vscale x 1 x i8> %1, <vscale x 1 x i8> %2, iXLen %3) nounwind {
 ; CHECK-LABEL: test2:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    vsetvli zero, a0, e8, mf8, ta, mu
-; CHECK-NEXT:    vmset.m v0
-; CHECK-NEXT:    vsetvli zero, zero, e8, mf8, tu, mu
-; CHECK-NEXT:    vadd.vv v8, v9, v10, v0.t
+; CHECK-NEXT:    vsetvli zero, a0, e8, mf8, tu, mu
+; CHECK-NEXT:    vadd.vv v8, v9, v10
 ; CHECK-NEXT:    ret
 entry:
   %allone = call <vscale x 1 x i1> @llvm.riscv.vmset.nxv1i1(
     iXLen %3);
   %a = call <vscale x 1 x i8> @llvm.riscv.vadd.mask.nxv1i8.nxv1i8(
     <vscale x 1 x i8> %0,
     <vscale x 1 x i8> %1,
     <vscale x 1 x i8> %2,