[RISCV] Fix IMPLICIT_DEF check in doPeepholeMaskedRVV

[luke]

Summary

An IMPLICIT_DEF is different from an undef node, so this patch fixes the
check for it.
It doesn't actually change any test output, as previously it was
conservatively emitting tail undisturbed with an IMPLICIT_DEF passthru,
which ultimately got selected as tail agnostic anyway.

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3194

This code is actually a bit suspect on a deeper level. What we call TA pseudos don't have a merge operand. As a result, they're actually tail undefined, not tail agnostic. I think we should be defaulting to using the TU form unless the pass through is implicit_def.

[luke]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3194

Yes, I thought the same as well and tried changing it to be TU by default, but got some strange test changes. Will investigate it a bit further anyway.

[luke]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

3188

@reames The more I look at this the more confused I get, specifically this hasVecPolicyOp check here.
Looking at computeInfoForInstr in RISCVInsertVSETVLI.cpp would imply that if a _MASK pseudo doesn't have a policy operand, then it's implicitly tail undisturbed and mask undisturbed.

However as you noted, if we don't a policy operand, then UseTUPseudo is set to false and we end up using UnmaskedPseudo, which is always tail undefined and mask undefined. And it doesn't have a merge operand, so we end up throwing away N's merge operand without actually checking if it's implicit_def. Which would be incorrect?

[reames]

JFYI, I deleted the undef check in c4a3bd7f.

Also, I'm now 100% convinced the line just above (the policy check) is a miscompile. It's probably a silent one, but it's definitely a miscompile since we haven't checked whether there are any tail elements (for which agnostic might be undefined).

[luke]

In D152810#4422399, @reames wrote:

JFYI, I deleted the undef check in c4a3bd7f.

Also, I'm now 100% convinced the line just above (the policy check) is a miscompile. It's probably a silent one, but it's definitely a miscompile since we haven't checked whether there are any tail elements (for which agnostic might be undefined).

Indeed. Also at least when running the codegen checks and compiling sqlite, the

if (I->UnmaskedTUPseudo == I->MaskedPseudo)
  return false;

branch seems to be dead too. Perhaps it should be an assertion?
I have a series of patches almost ready to fix the policy check, and as expected it seems to affect vmseq and vmflt.vv pseudos, i.e. mask pseudos without a policy op. Before I post it though I need to check that the test diff is actually correct.

[luke]

Superseded by https://reviews.llvm.org/D152963

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

if (!usesAllOnesMask(N, MaskOpIdx))
     return false;
 
   // Retrieve the tail policy operand index, if any.
   std::optional<unsigned> TailPolicyOpIdx;
   const RISCVInstrInfo &TII = *Subtarget->getInstrInfo();
   const MCInstrDesc &MaskedMCID = TII.get(N->getMachineOpcode());
 
   bool UseTUPseudo = false;
   if (RISCVII::hasVecPolicyOp(MaskedMCID.TSFlags)) {
     // Some operations are their own TU.
     if (I->UnmaskedTUPseudo == I->UnmaskedPseudo) {
       UseTUPseudo = true;
     } else {
       TailPolicyOpIdx = getVecPolicyOpIdx(N, MaskedMCID);
       if (!(N->getConstantOperandVal(*TailPolicyOpIdx) &
             RISCVII::TAIL_AGNOSTIC)) {
         // We can't use TA if the tie-operand is not IMPLICIT_DEF
-        if (!N->getOperand(0).isUndef()) {
+        if (!(N->getOperand(0).isMachineOpcode() &&
+              N->getOperand(0).getMachineOpcode() ==
+                  TargetOpcode::IMPLICIT_DEF)) {
           // Keep the true-masked instruction when there is no unmasked TU
           // instruction
           if (I->UnmaskedTUPseudo == I->MaskedPseudo)
             return false;
           UseTUPseudo = true;
         }
       }
     }