TII: Generalize X86's isSafeToClobberEFLAGs

[arsenm]

Fix handling of sub registers and live ins

[arsenm]

ping

[MatzeB]

Is this different from MachineBasicBlock::computeRegisterLiveness(TRI, Reg, I, 4) == LQR_Dead?
The implementations looks the same to me except that this one here has one more case where it checks the successor blocks live in lists...

[MatzeB]

Also I'd generally recommend to rather look at LivePhysReg/LiveRegUnits for doing liveness queries post-ra. They can be more expensive but give more consistent results...

[arsenm]

In D49401#1177010, @MatzeB wrote:

Also I'd generally recommend to rather look at LivePhysReg/LiveRegUnits for doing liveness queries post-ra. They can be more expensive but give more consistent results...

I want to use this in an SSA pass, not post-RA

[MatzeB]

In D49401#1177039, @arsenm wrote:

In D49401#1177010, @MatzeB wrote:

Also I'd generally recommend to rather look at LivePhysReg/LiveRegUnits for doing liveness queries post-ra. They can be more expensive but give more consistent results...

I want to use this in an SSA pass, not post-RA

Fair enough. But MachineBasicBlock::computeRegisterLiveness(TRI, Reg, I, 4) == LQR_Dead? should work in machine SSA, shouldn't it?

[arsenm]

In D49401#1206110, @MatzeB wrote:

In D49401#1177039, @arsenm wrote:

In D49401#1177010, @MatzeB wrote:

Also I'd generally recommend to rather look at LivePhysReg/LiveRegUnits for doing liveness queries post-ra. They can be more expensive but give more consistent results...

I want to use this in an SSA pass, not post-RA

Fair enough. But MachineBasicBlock::computeRegisterLiveness(TRI, Reg, I, 4) == LQR_Dead? should work in machine SSA, shouldn't it?

I tried swapping these and found a few bugs in each.

isSafeToClobber doesn't count debug instruction towards the search count, while computeRegisterLiveness does which is just a bug. I also found another subregister bug in isSafeToClobber. computeRegisterLiveness seems overall nicer, but might need some changes.

The most important difference seems to be the handling of unused defs that might be live out. If a physical register is defined and not used, computeRegisterLiveness says it's alive and doesn't consider that it's not live in to any successors. I suppose this could be solved by searching uses before defs, and adding another liveness type LQR_LiveUnused or something? I suppose other users might care that the register has any value.

[arsenm]

With D51348 and D51350 I suspect X86 can switch to just using computeRegisterLiveness but I haven't tried it yet

[arsenm]

Don't need this anymore, but x86 should probably remove this function

include/llvm/CodeGen/TargetInstrInfo.h

public:
   ///
   /// \note The generic implementation does not provide any support for
   /// MI.isInsertSubregLike(). In other words, one has to override
   /// getInsertSubregLikeInputs for target specific instructions.
   bool getInsertSubregInputs(const MachineInstr &MI, unsigned DefIdx,
                              RegSubRegPair &BaseReg,
                              RegSubRegPairAndIdx &InsertedReg) const;
 
+  /// \returns true if it's safe insert an instruction that would clobber the \p
+  /// Reg physical register. Note the result may be conservative. If it cannot
+  /// definitely determine the safety after visiting \p MaxInstructions
+  /// instructions in each direction it assumes it's not safe.
+  bool isSafeToClobberReg(MachineBasicBlock &MBB,
+                          MachineBasicBlock::iterator I,
+                          const TargetRegisterInfo &TRI,
+                          unsigned Reg,
+                          unsigned MaxInstructions) const;
+
   /// Return true if two machine instructions would produce identical values.
   /// By default, this is only true when the two instructions
   /// are deemed identical except for defs. If this function is called when the
   /// IR is still in SSA form, the caller can pass the MachineRegisterInfo for
   /// aggressive checks.
   virtual bool produceSameValue(const MachineInstr &MI0,
                                 const MachineInstr &MI1,
                                 const MachineRegisterInfo *MRI = nullptr) const;

lib/CodeGen/TargetInstrInfo.cpp

bool TargetInstrInfo::getInsertSubregInputs(
   BaseReg.Reg = MOBaseReg.getReg();
   BaseReg.SubReg = MOBaseReg.getSubReg();
 
   InsertedReg.Reg = MOInsertedReg.getReg();
   InsertedReg.SubReg = MOInsertedReg.getSubReg();
   InsertedReg.SubIdx = (unsigned)MOSubIdx.getImm();
   return true;
 }
+
+bool TargetInstrInfo::isSafeToClobberReg(MachineBasicBlock &MBB,
+                                         MachineBasicBlock::iterator I,
+                                         const TargetRegisterInfo &TRI,
+                                         const unsigned Reg,
+                                         unsigned MaxInstructions) const {
+  assert(TargetRegisterInfo::isPhysicalRegister(Reg) &&
+         "this is only for physical registers");
+
+  MachineBasicBlock::iterator E = MBB.end();
+
+  // For compile time consideration, if we are not able to determine the
+  // safety after visiting 4 instructions in each direction, we will assume
+  // it's not safe.
+  MachineBasicBlock::iterator Iter = I;
+  for (unsigned i = 0; Iter != E && i < MaxInstructions; ++i) {
+    bool SeenDef = false;
+    for (unsigned j = 0, e = Iter->getNumOperands(); j != e; ++j) {
+      MachineOperand &MO = Iter->getOperand(j);
+      if (MO.isRegMask() && MO.clobbersPhysReg(Reg))
+        SeenDef = true;
+      if (!MO.isReg())
+        continue;
+      if (MO.getReg() == Reg) {
+        if (MO.isUse())
+          return false;
+        SeenDef = true;
+      }
+    }
+
+    if (SeenDef) {
+      // This instruction defines Reg, no need to look any further.
+      return true;
+    }
+
+    ++Iter;
+    // Skip over debug instructions.
+    while (Iter != E && Iter->isDebugInstr())
+      ++Iter;
+  }
+
+  // It is safe to clobber Reg at the end of a block of no successor has it
+  // live in.
+  if (Iter == E) {
+    for (MachineBasicBlock *S : MBB.successors()) {
+      for (MCSubRegIterator SubReg(Reg, &TRI, /*IncludeSelf*/true);
+           SubReg.isValid(); ++SubReg) {
+        if (S->isLiveIn(*SubReg))
+          return false;
+      }
+    }
+
+    return true;
+  }
+
+  MachineBasicBlock::iterator B = MBB.begin();
+  Iter = I;
+  for (unsigned i = 0; i < MaxInstructions; ++i) {
+    // If we make it to the beginning of the block, it's safe to clobber
+    // Reg iff Reg is not live-in.
+    if (Iter == B) {
+      for (MCSubRegIterator SubReg(Reg, &TRI, /*IncludeSelf*/true);
+           SubReg.isValid(); ++SubReg) {
+        if (MBB.isLiveIn(Reg))
+          return false;
+      }
+
+      return true;
+    }
+
+    --Iter;
+    // Skip over debug instructions.
+    while (Iter != B && Iter->isDebugInstr())
+      --Iter;
+
+    bool SawKill = false;
+    for (unsigned j = 0, e = Iter->getNumOperands(); j != e; ++j) {
+      MachineOperand &MO = Iter->getOperand(j);
+      // A register mask may clobber Reg, but we should still look for a
+      // live Reg def.
+      if (MO.isRegMask() && MO.clobbersPhysReg(Reg))
+        SawKill = true;
+      if (MO.isReg() && MO.getReg() == Reg) {
+        if (MO.isDef())
+          return MO.isDead();
+        if (MO.isKill())
+          SawKill = true;
+      }
+    }
+
+    if (SawKill) {
+      // This instruction kills Reg and doesn't redefine it, so
+      // there's no need to look further.
+      return true;
+    }
+  }
+
+  // Conservative answer.
+  return false;
+}

lib/Target/X86/X86InstrInfo.h

public:
   /// instruction that defines the specified register class.
   bool isSafeToMoveRegClassDefs(const TargetRegisterClass *RC) const override;
 
   /// isSafeToClobberEFLAGS - Return true if it's safe insert an instruction tha
   /// would clobber the EFLAGS condition register. Note the result may be
   /// conservative. If it cannot definitely determine the safety after visiting
   /// a few instructions in each direction it assumes it's not safe.
   bool isSafeToClobberEFLAGS(MachineBasicBlock &MBB,
-                             MachineBasicBlock::iterator I) const;
+                             MachineBasicBlock::iterator I) const {
+    return isSafeToClobberReg(MBB, I, RI, X86::EFLAGS, 4);
+  }
 
   /// True if MI has a condition code def, e.g. EFLAGS, that is
   /// not marked dead.
   bool hasLiveCondCodeDef(MachineInstr &MI) const;
 
   /// getGlobalBaseReg - Return a virtual register initialized with the
   /// the global base register value. Output instructions required to
   /// initialize the register in the function entry block, if necessary.

lib/Target/X86/X86InstrInfo.cpp

bool X86InstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
   }
   }
 
   // All other instructions marked M_REMATERIALIZABLE are always trivially
   // rematerializable.
   return true;
 }
 
-bool X86InstrInfo::isSafeToClobberEFLAGS(MachineBasicBlock &MBB,
-                                         MachineBasicBlock::iterator I) const {
-  MachineBasicBlock::iterator E = MBB.end();
-
-  // For compile time consideration, if we are not able to determine the
-  // safety after visiting 4 instructions in each direction, we will assume
-  // it's not safe.
-  MachineBasicBlock::iterator Iter = I;
-  for (unsigned i = 0; Iter != E && i < 4; ++i) {
-    bool SeenDef = false;
-    for (unsigned j = 0, e = Iter->getNumOperands(); j != e; ++j) {
-      MachineOperand &MO = Iter->getOperand(j);
-      if (MO.isRegMask() && MO.clobbersPhysReg(X86::EFLAGS))
-        SeenDef = true;
-      if (!MO.isReg())
-        continue;
-      if (MO.getReg() == X86::EFLAGS) {
-        if (MO.isUse())
-          return false;
-        SeenDef = true;
-      }
-    }
-
-    if (SeenDef)
-      // This instruction defines EFLAGS, no need to look any further.
-      return true;
-    ++Iter;
-    // Skip over debug instructions.
-    while (Iter != E && Iter->isDebugInstr())
-      ++Iter;
-  }
-
-  // It is safe to clobber EFLAGS at the end of a block of no successor has it
-  // live in.
-  if (Iter == E) {
-    for (MachineBasicBlock *S : MBB.successors())
-      if (S->isLiveIn(X86::EFLAGS))
-        return false;
-    return true;
-  }
-
-  MachineBasicBlock::iterator B = MBB.begin();
-  Iter = I;
-  for (unsigned i = 0; i < 4; ++i) {
-    // If we make it to the beginning of the block, it's safe to clobber
-    // EFLAGS iff EFLAGS is not live-in.
-    if (Iter == B)
-      return !MBB.isLiveIn(X86::EFLAGS);
-
-    --Iter;
-    // Skip over debug instructions.
-    while (Iter != B && Iter->isDebugInstr())
-      --Iter;
-
-    bool SawKill = false;
-    for (unsigned j = 0, e = Iter->getNumOperands(); j != e; ++j) {
-      MachineOperand &MO = Iter->getOperand(j);
-      // A register mask may clobber EFLAGS, but we should still look for a
-      // live EFLAGS def.
-      if (MO.isRegMask() && MO.clobbersPhysReg(X86::EFLAGS))
-        SawKill = true;
-      if (MO.isReg() && MO.getReg() == X86::EFLAGS) {
-        if (MO.isDef()) return MO.isDead();
-        if (MO.isKill()) SawKill = true;
-      }
-    }
-
-    if (SawKill)
-      // This instruction kills EFLAGS and doesn't redefine it, so
-      // there's no need to look further.
-      return true;
-  }
-
-  // Conservative answer.
-  return false;
-}
-
 void X86InstrInfo::reMaterialize(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator I,
                                  unsigned DestReg, unsigned SubIdx,
                                  const MachineInstr &Orig,
                                  const TargetRegisterInfo &TRI) const {
   bool ClobbersEFLAGS = false;
   for (const MachineOperand &MO : Orig.operands()) {
     if (MO.isReg() && MO.isDef() && MO.getReg() == X86::EFLAGS) {