[RISCV] Untangle instruction properties from VSETVLIInfo [NFC]

[reames]

Summary

The abstract state used in the data flow should not know anything about the instructions which produced the abstract states. Instead, when comparing two states, we can simply use information about the machine instr at that time.

In the old design, basically any use of the instruction flags on the current (as opposed to a "Require" - aka upcoming state) would be a bug. We don't seem to actually have any such bugs, but we can make this much more obvious with code structure.

[frasercrmck]

I agree that instruction properties shouldn't be part of the abstract state, so I agree with the principle behind this patch.

I worry that this change is throwing another free variable into the mix when checking compatibility, which causes confusion and potentially leads to another sort of subtle bug - or people worrying about another sort of bug - about what happens if MI and Require are out of sync somehow.

As far as I can tell, MI and Require have to be in sync for correctness, is that right? We only ever call needVSETVLI when the Require has been computed from MI, anyway. So one idea I had was: may we have a tighter implementation if we removed the Require parameter from needVSETVLI and computed it fresh inside that function from MI? That way its API is obvious: "does this MI need a VSETVLI given the current VSETVLI state?". Now, we do need Require in other places outside of needVSETVLI so I dunno if it would just be moving things around as we'd be computing it twice - inside and out - rather than just the once.

Another option - could we assert that the Require is equivalent to that of a freshly-computed VSETVLIInfo?

Or we just document it in needVSETVLI and other APIs somehow. I see you've done that in isCompatible but I still worry about bugs if we're simply documenting rather than ensuring/checking/asserting.

[kito-cheng]

I think the real problem is we didn't extract the abstract info from MI for those case, so what do you think if we turn those info into more abstraction level info and still keep in Require?

e.g.
MaskRegOp: Require same ratio (VLMAX) but no matter LMUL and SEW.
StoreOp: No need tail/mask policy, (Handled by canSkipVSETVLIForLoadStore for now) Require same ratio (VLMAX) but no matter LMUL and SEW, because EEW.
ScalarMovOp: Require same SEW but no matter the LMUL, and always tail-agnostic regardless VTYPE setting
LoadOp: (Handled by canSkipVSETVLIForLoadStore for now) Require same ratio (VLMAX) but no matter LMUL and SEW, because EEW.
Any operation with mask destination: tail elements are always treated as tail-agnostic, regardless of the setting of vta.

So the abstract info is:

• No matter LMUL?
• Require same ratio - no matter LMUL and SEW.
• Regardless tail policy of the setting of VTYPE?
• Regardless mask policy of the setting of VTYPE?

And then we can get rid of MI once we gather those info into Require.

[reames]

@frasercrmck Let's not let perfection be the enemy of the good here. I see the point you're raising, and I'm willing to explore it in a follow up patch, but can we get this one in first? I see it as a significant improvement in the current code.

Edit: To clarify, the reason this is tricky is that we call needVSETVLI *after mutating MI* in phase3 - specifically, we clear VLOpReg. As a result, the trivially assert fails because the newly computed info disagrees with the info before mutation. Introducing even the assert thus requires a larger code reorg than it seems like it should.

@kito-cheng No, I will not do as you propose. These are properties of the instruction, trying to abstract them into an otherwise unrelated structure just makes the code confusing and hard to follow. For clarity, I'm not objecting to maybe abstracting the policy bits (if we find other examples which share the same policies), I'm objecting to them being in the abstract state used in the forward dataflow. These are *backwards* (e.g. demanded) facts. If we had a bidirectional dataflow here, maybe merging them would make sense, but we don't.

[frasercrmck]

@reames Fair enough, but I still think I'd like to see a comment on needVSETVLI to say MI and Require must be in sync.

[reames]

Comment updated per request.

p.s. @frasercrmck, you commented at about the same time I edited my previous comment for clarity. If you haven't seen that, might want to read the part starting with Edit:

[frasercrmck]

LGTM, thanks! About your edit, yeah, I was wondering/worrying if that mutation was going to affect things. Drat...

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp

static cl::opt<bool> DisableInsertVSETVLPHIOpt(
     cl::desc("Disable looking through phis when inserting vsetvlis."));
 
 static cl::opt<bool> UseStrictAsserts(
     "riscv-insert-vsetvl-strict-asserts", cl::init(true), cl::Hidden,
     cl::desc("Enable strict assertion checking for the dataflow algorithm"));
 
 namespace {
 
+static unsigned getVLOpNum(const MachineInstr &MI) {
+  return RISCVII::getVLOpNum(MI.getDesc());
+}
+
+static unsigned getSEWOpNum(const MachineInstr &MI) {
+  return RISCVII::getSEWOpNum(MI.getDesc());
+}
+
+static bool isScalarMoveInstr(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  default:
+    return false;
+  case RISCV::PseudoVMV_S_X_M1:
+  case RISCV::PseudoVMV_S_X_M2:
+  case RISCV::PseudoVMV_S_X_M4:
+  case RISCV::PseudoVMV_S_X_M8:
+  case RISCV::PseudoVMV_S_X_MF2:
+  case RISCV::PseudoVMV_S_X_MF4:
+  case RISCV::PseudoVMV_S_X_MF8:
+  case RISCV::PseudoVFMV_S_F16_M1:
+  case RISCV::PseudoVFMV_S_F16_M2:
+  case RISCV::PseudoVFMV_S_F16_M4:
+  case RISCV::PseudoVFMV_S_F16_M8:
+  case RISCV::PseudoVFMV_S_F16_MF2:
+  case RISCV::PseudoVFMV_S_F16_MF4:
+  case RISCV::PseudoVFMV_S_F32_M1:
+  case RISCV::PseudoVFMV_S_F32_M2:
+  case RISCV::PseudoVFMV_S_F32_M4:
+  case RISCV::PseudoVFMV_S_F32_M8:
+  case RISCV::PseudoVFMV_S_F32_MF2:
+  case RISCV::PseudoVFMV_S_F64_M1:
+  case RISCV::PseudoVFMV_S_F64_M2:
+  case RISCV::PseudoVFMV_S_F64_M4:
+  case RISCV::PseudoVFMV_S_F64_M8:
+    return true;
+  }
+}
+
+
 class VSETVLIInfo {
   union {
     Register AVLReg;
     unsigned AVLImm;
   };
 
   enum : uint8_t {
     Uninitialized,
     AVLIsReg,
     AVLIsImm,
     Unknown,
   } State = Uninitialized;
 
   // Fields from VTYPE.
   RISCVII::VLMUL VLMul = RISCVII::LMUL_1;
   uint8_t SEW = 0;
   uint8_t TailAgnostic : 1;
   uint8_t MaskAgnostic : 1;
-  uint8_t MaskRegOp : 1;
-  uint8_t StoreOp : 1;
-  uint8_t ScalarMovOp : 1;
   uint8_t SEWLMULRatioOnly : 1;
 
 public:
   VSETVLIInfo()
-      : AVLImm(0), TailAgnostic(false), MaskAgnostic(false), MaskRegOp(false),
-        StoreOp(false), ScalarMovOp(false), SEWLMULRatioOnly(false) {}
+      : AVLImm(0), TailAgnostic(false), MaskAgnostic(false),
+        SEWLMULRatioOnly(false) {}
 
   static VSETVLIInfo getUnknown() {
     VSETVLIInfo Info;
     Info.setUnknown();
     return Info;
   }
 
   bool isValid() const { return State != Uninitialized; }
public:
   void setVTYPE(unsigned VType) {
     assert(isValid() && !isUnknown() &&
            "Can't set VTYPE for uninitialized or unknown");
     VLMul = RISCVVType::getVLMUL(VType);
     SEW = RISCVVType::getSEW(VType);
     TailAgnostic = RISCVVType::isTailAgnostic(VType);
     MaskAgnostic = RISCVVType::isMaskAgnostic(VType);
   }
-  void setVTYPE(RISCVII::VLMUL L, unsigned S, bool TA, bool MA, bool MRO,
-                bool IsStore, bool IsScalarMovOp) {
+  void setVTYPE(RISCVII::VLMUL L, unsigned S, bool TA, bool MA) {
     assert(isValid() && !isUnknown() &&
            "Can't set VTYPE for uninitialized or unknown");
     VLMul = L;
     SEW = S;
     TailAgnostic = TA;
     MaskAgnostic = MA;
-    MaskRegOp = MRO;
-    StoreOp = IsStore;
-    ScalarMovOp = IsScalarMovOp;
   }
 
   unsigned encodeVTYPE() const {
     assert(isValid() && !isUnknown() && !SEWLMULRatioOnly &&
            "Can't encode VTYPE for uninitialized or unknown");
     return RISCVVType::encodeVTYPE(VLMul, SEW, TailAgnostic, MaskAgnostic);
   }
 
bool hasSamePolicy(const VSETVLIInfo &Other) const {
     assert(isValid() && Other.isValid() &&
            "Can't compare invalid VSETVLIInfos");
     assert(!isUnknown() && !Other.isUnknown() &&
            "Can't compare VTYPE in unknown state");
     return TailAgnostic == Other.TailAgnostic &&
            MaskAgnostic == Other.MaskAgnostic;
   }
 
-  bool hasCompatibleVTYPE(const VSETVLIInfo &Require) const {
+  bool hasCompatibleVTYPE(const MachineInstr &MI, const VSETVLIInfo &Require) const {
     // Simple case, see if full VTYPE matches.
     if (hasSameVTYPE(Require))
       return true;
 
     // If this is a mask reg operation, it only cares about VLMAX.
     // FIXME: Mask reg operations are probably ok if "this" VLMAX is larger
     // than "Require".
     // FIXME: The policy bits can probably be ignored for mask reg operations.
-    if (Require.MaskRegOp && hasSameVLMAX(Require) &&
+    const unsigned Log2SEW = MI.getOperand(getSEWOpNum(MI)).getImm();
+    // A Log2SEW of 0 is an operation on mask registers only.
+    const bool MaskRegOp = Log2SEW == 0;
+    if (MaskRegOp && hasSameVLMAX(Require) &&
         TailAgnostic == Require.TailAgnostic &&
         MaskAgnostic == Require.MaskAgnostic)
       return true;
 
     return false;
   }
 
   // Determine whether the vector instructions requirements represented by
   // Require are compatible with the previous vsetvli instruction represented
-  // by this.
-  bool isCompatible(const VSETVLIInfo &Require) const {
+  // by this.  MI is the instruction whose requirements we're considering.
+  bool isCompatible(const MachineInstr &MI, const VSETVLIInfo &Require) const {
     assert(isValid() && Require.isValid() &&
            "Can't compare invalid VSETVLIInfos");
     assert(!Require.SEWLMULRatioOnly &&
            "Expected a valid VTYPE for instruction!");
     // Nothing is compatible with Unknown.
     if (isUnknown() || Require.isUnknown())
       return false;
 
     // If only our VLMAX ratio is valid, then this isn't compatible.
     if (SEWLMULRatioOnly)
       return false;
 
     // If the instruction doesn't need an AVLReg and the SEW matches, consider
     // it compatible.
     if (Require.hasAVLReg() && Require.AVLReg == RISCV::NoRegister)
       if (SEW == Require.SEW)
         return true;
 
     // For vmv.s.x and vfmv.s.f, there is only two behaviors, VL = 0 and VL > 0.
     // So it's compatible when we could make sure that both VL be the same
     // situation.
-    if (Require.ScalarMovOp && Require.hasAVLImm() &&
+    if (isScalarMoveInstr(MI) && Require.hasAVLImm() &&
         ((hasNonZeroAVL() && Require.hasNonZeroAVL()) ||
          (hasZeroAVL() && Require.hasZeroAVL())) &&
         hasSameSEW(Require) && hasSamePolicy(Require))
       return true;
 
     // The AVL must match.
     if (!hasSameAVL(Require))
       return false;
 
-    if (hasCompatibleVTYPE(Require))
+    if (hasCompatibleVTYPE(MI, Require))
       return true;
 
     // Store instructions don't use the policy fields.
-    // TODO: Move into hasCompatibleVTYPE?
-    if (Require.StoreOp && VLMul == Require.VLMul && SEW == Require.SEW)
+    const bool StoreOp = MI.getNumExplicitDefs() == 0;
+    if (StoreOp && VLMul == Require.VLMul && SEW == Require.SEW)
       return true;
 
     // Anything else is not compatible.
     return false;
   }
 
   bool isCompatibleWithLoadStoreEEW(unsigned EEW,
                                     const VSETVLIInfo &Require) const {
     assert(isValid() && Require.isValid() &&
            "Can't compare invalid VSETVLIInfos");
     assert(!Require.SEWLMULRatioOnly &&
            "Expected a valid VTYPE for instruction!");
     assert(EEW == Require.SEW && "Mismatched EEW/SEW for store");
 
     if (isUnknown() || hasSEWLMULRatioOnly())
       return false;
 
     if (!hasSameAVL(Require))
       return false;
 
-    // Stores can ignore the tail and mask policies.
-    if (!Require.StoreOp && (TailAgnostic != Require.TailAgnostic ||
-                               MaskAgnostic != Require.MaskAgnostic))
-      return false;
-
     return getSEWLMULRatio() == getSEWLMULRatio(EEW, Require.VLMul);
   }
 
   bool operator==(const VSETVLIInfo &Other) const {
     // Uninitialized is only equal to another Uninitialized.
     if (!isValid())
       return !Other.isValid();
     if (!Other.isValid())
if (hasAVLReg())
       OS << "AVLReg=" << (unsigned)AVLReg;
     if (hasAVLImm())
       OS << "AVLImm=" << (unsigned)AVLImm;
     OS << ", "
        << "VLMul=" << (unsigned)VLMul << ", "
        << "SEW=" << (unsigned)SEW << ", "
        << "TailAgnostic=" << (bool)TailAgnostic << ", "
        << "MaskAgnostic=" << (bool)MaskAgnostic << ", "
-       << "MaskRegOp=" << (bool)MaskRegOp << ", "
-       << "StoreOp=" << (bool)StoreOp << ", "
-       << "ScalarMovOp=" << (bool)ScalarMovOp << ", "
        << "SEWLMULRatioOnly=" << (bool)SEWLMULRatioOnly << "}";
   }
 #endif
 };
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_ATTRIBUTE_USED
 inline raw_ostream &operator<<(raw_ostream &OS, const VSETVLIInfo &V) {
while (true) {
     if (!Register::isVirtualRegister(MI->getOperand(1).getReg()))
       return nullptr;
     MI = MRI->getVRegDef(MI->getOperand(1).getReg());
     if (!MI)
       return nullptr;
   }
 }
 
-static bool isScalarMoveInstr(const MachineInstr &MI) {
-  switch (MI.getOpcode()) {
-  default:
-    return false;
-  case RISCV::PseudoVMV_S_X_M1:
-  case RISCV::PseudoVMV_S_X_M2:
-  case RISCV::PseudoVMV_S_X_M4:
-  case RISCV::PseudoVMV_S_X_M8:
-  case RISCV::PseudoVMV_S_X_MF2:
-  case RISCV::PseudoVMV_S_X_MF4:
-  case RISCV::PseudoVMV_S_X_MF8:
-  case RISCV::PseudoVFMV_S_F16_M1:
-  case RISCV::PseudoVFMV_S_F16_M2:
-  case RISCV::PseudoVFMV_S_F16_M4:
-  case RISCV::PseudoVFMV_S_F16_M8:
-  case RISCV::PseudoVFMV_S_F16_MF2:
-  case RISCV::PseudoVFMV_S_F16_MF4:
-  case RISCV::PseudoVFMV_S_F32_M1:
-  case RISCV::PseudoVFMV_S_F32_M2:
-  case RISCV::PseudoVFMV_S_F32_M4:
-  case RISCV::PseudoVFMV_S_F32_M8:
-  case RISCV::PseudoVFMV_S_F32_MF2:
-  case RISCV::PseudoVFMV_S_F64_M1:
-  case RISCV::PseudoVFMV_S_F64_M2:
-  case RISCV::PseudoVFMV_S_F64_M4:
-  case RISCV::PseudoVFMV_S_F64_M8:
-    return true;
-  }
-}
-
-static unsigned getVLOpNum(const MachineInstr &MI) {
-  return RISCVII::getVLOpNum(MI.getDesc());
-}
-
-static unsigned getSEWOpNum(const MachineInstr &MI) {
-  return RISCVII::getSEWOpNum(MI.getDesc());
-}
-
 static VSETVLIInfo computeInfoForInstr(const MachineInstr &MI, uint64_t TSFlags,
                                        const MachineRegisterInfo *MRI) {
   VSETVLIInfo InstrInfo;
 
   // If the instruction has policy argument, use the argument.
   // If there is no policy argument, default to tail agnostic unless the
   // destination is tied to a source. Unless the source is undef. In that case
   // the user would have some control over the policy values.
if (RISCVII::hasVecPolicyOp(TSFlags)) {
     if (RISCVII::doesForceTailAgnostic(TSFlags))
       TailAgnostic = true;
   }
 
   RISCVII::VLMUL VLMul = RISCVII::getLMul(TSFlags);
 
   unsigned Log2SEW = MI.getOperand(getSEWOpNum(MI)).getImm();
   // A Log2SEW of 0 is an operation on mask registers only.
-  bool MaskRegOp = Log2SEW == 0;
   unsigned SEW = Log2SEW ? 1 << Log2SEW : 8;
   assert(RISCVVType::isValidSEW(SEW) && "Unexpected SEW");
 
-  // If there are no explicit defs, this is a store instruction which can
-  // ignore the tail and mask policies.
-  bool StoreOp = MI.getNumExplicitDefs() == 0;
-  bool ScalarMovOp = isScalarMoveInstr(MI);
-
   if (RISCVII::hasVLOp(TSFlags)) {
     const MachineOperand &VLOp = MI.getOperand(getVLOpNum(MI));
     if (VLOp.isImm()) {
       int64_t Imm = VLOp.getImm();
       // Conver the VLMax sentintel to X0 register.
       if (Imm == RISCV::VLMaxSentinel)
         InstrInfo.setAVLReg(RISCV::X0);
       else
         InstrInfo.setAVLImm(Imm);
     } else {
       InstrInfo.setAVLReg(VLOp.getReg());
     }
   } else {
     InstrInfo.setAVLReg(RISCV::NoRegister);
   }
-  InstrInfo.setVTYPE(VLMul, SEW, TailAgnostic, MaskAgnostic, MaskRegOp, StoreOp,
-                     ScalarMovOp);
+  InstrInfo.setVTYPE(VLMul, SEW, TailAgnostic, MaskAgnostic);
 
   return InstrInfo;
 }
 
 void RISCVInsertVSETVLI::insertVSETVLI(MachineBasicBlock &MBB, MachineInstr &MI,
                                        const VSETVLIInfo &Info,
                                        const VSETVLIInfo &PrevInfo) {
   DebugLoc DL = MI.getDebugLoc();
bool canSkipVSETVLIForLoadStore(const MachineInstr &MI,
   case RISCV::PseudoVSSE64_V_M4:
   case RISCV::PseudoVSSE64_V_M4_MASK:
   case RISCV::PseudoVSSE64_V_M8:
   case RISCV::PseudoVSSE64_V_M8_MASK:
     EEW = 64;
     break;
   }
 
+  // Stores can ignore the tail and mask policies.
+  const bool StoreOp = MI.getNumExplicitDefs() == 0;
+  if (!StoreOp && !CurInfo.hasSamePolicy(Require))
+    return false;
+
   return CurInfo.isCompatibleWithLoadStoreEEW(EEW, Require);
 }
 
+/// Return true if a VSETVLI is required to transition from CurInfo to Require
+/// before MI.  Require corresponds to the result of computeInfoForInstr(MI...)
+/// *before* we clear VLOp in phase3.  We can't recompute and assert it here due
+/// to that muation.
 bool RISCVInsertVSETVLI::needVSETVLI(const MachineInstr &MI, const VSETVLIInfo &Require,
                                      const VSETVLIInfo &CurInfo) const {
-  if (CurInfo.isCompatible(Require))
+  if (CurInfo.isCompatible(MI, Require))
     return false;
 
   // We didn't find a compatible value. If our AVL is a virtual register,
   // it might be defined by a VSET(I)VLI. If it has the same VLMAX we need
   // and the last VL/VTYPE we observed is the same, we don't need a
   // VSETVLI here.
   if (!CurInfo.isUnknown() && Require.hasAVLReg() &&
       Require.getAVLReg().isVirtual() && !CurInfo.hasSEWLMULRatioOnly() &&
-      CurInfo.hasCompatibleVTYPE(Require)) {
+      CurInfo.hasCompatibleVTYPE(MI, Require)) {
     if (MachineInstr *DefMI = MRI->getVRegDef(Require.getAVLReg())) {
       if (isVectorConfigInstr(*DefMI)) {
         VSETVLIInfo DefInfo = getInfoForVSETVLI(*DefMI);
         if (DefInfo.hasSameAVL(CurInfo) && DefInfo.hasSameVLMAX(CurInfo))
           return false;
       }
     }
   }
 
   // If this is a unit-stride or strided load/store, we may be able to use the
   // EMUL=(EEW/SEW)*LMUL relationship to avoid changing VTYPE.
-  return !canSkipVSETVLIForLoadStore(MI, Require, CurInfo);
+  return CurInfo.isUnknown() || !canSkipVSETVLIForLoadStore(MI, Require, CurInfo);
 }
 
 bool RISCVInsertVSETVLI::computeVLVTYPEChanges(const MachineBasicBlock &MBB) {
   bool HadVectorOp = false;
 
   BlockData &BBInfo = BlockInfo[MBB.getNumber()];
   BBInfo.Change = BBInfo.Pred;
   for (const MachineInstr &MI : MBB) {
bool RISCVInsertVSETVLI::needVSETVLIPHI(const VSETVLIInfo &Require,
 
   for (unsigned PHIOp = 1, NumOps = PHI->getNumOperands(); PHIOp != NumOps;
        PHIOp += 2) {
     Register InReg = PHI->getOperand(PHIOp).getReg();
     MachineBasicBlock *PBB = PHI->getOperand(PHIOp + 1).getMBB();
     const BlockData &PBBInfo = BlockInfo[PBB->getNumber()];
     // If the exit from the predecessor has the VTYPE we are looking for
     // we might be able to avoid a VSETVLI.
-    if (PBBInfo.Exit.isUnknown() || !PBBInfo.Exit.hasCompatibleVTYPE(Require))
+    if (PBBInfo.Exit.isUnknown() || !PBBInfo.Exit.hasSameVTYPE(Require))
       return true;
 
     // We need the PHI input to the be the output of a VSET(I)VLI.
     MachineInstr *DefMI = MRI->getVRegDef(InReg);
     if (!DefMI || !isVectorConfigInstr(*DefMI))
       return true;
 
     // We found a VSET(I)VLI make sure it matches the output of the