[x86][SLH] Rm liveness check from data invariance check

[zbrid]

Summary

SLH had two functions named isDataInvariant and isDataInvariantLoad that
checked whether the passed instruction was data invariant. For some instructions,
if the EFLAGS were dead then they were considered data invariant, otherwise
they were not considered data invariant.

In this patch, I extracted that EFLAGS liveness check and made it
explicit at every call to isDataInvariant and isDataInvariantLoad.
This makes the isDataInvariant function behave more generally
and preserves the liveness check behavior that SLH would like to have.

Tested via llvm-lit llvm/test/CodeGen/X86/speculative-load-hardening*

This is the first step in making these two data invariance checks
available for non-SLH passes. The second step is to move the passes from
SLH to X86InstrInfo.cpp. I'll follow up with a patch that does that.

[zbrid]

[x86][SLH] Update commit message for clarity

[rnk]

lgtm

I was waiting for input from Chandler, but in the absence of that (one month later >_>), I see that this is NFC, and you are really working to make isDataInvariant(Load) shared with other passes. This seems like a step in that direction, and it all makes sense. I think you can go ahead and land this.

[zbrid]

Rebased and tested. No change to the code.

[zbrid]

Uh dang. I messed this up. I'm going to close this and make a new change that only removes the liveness check.

[zbrid]

Oh wait, no this looks fine. Woo hoo.

[zbrid]

No changes.

[MaskRay]

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

2222

Delete excess parentheses.

[zbrid]

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

2222

Hm, I'm not sure which parentheses you mean. On my scan, I don't see any extras.

[zbrid]

Welp, I actually did mess this up. See this for the fix: https://reviews.llvm.org/D75650 Fortunately, it's a NFC to fix the mess up, that is, the code works correctly either way.

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

static bool isDataInvariantLoad(MachineInstr &MI) {
   case X86::MOVZX32rm8:
   case X86::MOVZX32rm8_NOREX:
   case X86::MOVZX64rm16:
   case X86::MOVZX64rm8:
     return true;
   }
 }
 
+// Returns true if the MI has EFLAGS as a register def operand and it's live,
+// otherwise it returns false
+static bool isEFLAGSDefLive(const MachineInstr &MI) {
+  if (const MachineOperand *DefOp = MI.findRegisterDefOperand(X86::EFLAGS)) {
+    return !DefOp->isDead();
+  }
+  return false;
+}
+
 static bool isEFLAGSLive(MachineBasicBlock &MBB, MachineBasicBlock::iterator I,
                          const TargetRegisterInfo &TRI) {
   // Check if EFLAGS are alive by seeing if there is a def of them or they
   // live-in, and then seeing if that def is in turn used.
   for (MachineInstr &MI : llvm::reverse(llvm::make_range(MBB.begin(), I))) {
     if (MachineOperand *DefOp = MI.findRegisterDefOperand(X86::EFLAGS)) {
       // If the def is dead, then EFLAGS is not live.
       if (DefOp->isDead())
if (HardenLoads)
           continue;
 
         // If post-load hardening is enabled, this load is compatible with
         // post-load hardening, and we aren't already going to harden one of the
         // address registers, queue it up to be hardened post-load. Notably,
         // even once hardened this won't introduce a useful dependency that
         // could prune out subsequent loads.
         if (EnablePostLoadHardening && isDataInvariantLoad(MI) &&
-            MI.getDesc().getNumDefs() == 1 && MI.getOperand(0).isReg() &&
+            !isEFLAGSDefLive(MI) && MI.getDesc().getNumDefs() == 1 &&
+            MI.getOperand(0).isReg() &&
             canHardenRegister(MI.getOperand(0).getReg()) &&
             !HardenedAddrRegs.count(BaseReg) &&
             !HardenedAddrRegs.count(IndexReg)) {
           HardenPostLoad.insert(&MI);
           HardenedAddrRegs.insert(MI.getOperand(0).getReg());
           continue;
         }
 
for (MachineInstr &MI : MBB) {
           continue;
         }
 
         // Test if this instruction is one of our post load instructions (and
         // remove it from the set if so).
         if (HardenPostLoad.erase(&MI)) {
           assert(!MI.isCall() && "Must not try to post-load harden a call!");
 
-          // If this is a data-invariant load, we want to try and sink any
-          // hardening as far as possible.
-          if (isDataInvariantLoad(MI)) {
+          // If this is a data-invariant load and there is no EFLAGS
+          // interference, we want to try and sink any hardening as far as
+          // possible.
+          if (isDataInvariantLoad(MI) && !isEFLAGSDefLive(MI)) {
             // Sink the instruction we'll need to harden as far as we can down
             // the graph.
             MachineInstr *SunkMI = sinkPostLoadHardenedInst(MI, HardenPostLoad);
 
             // If we managed to sink this instruction, update everything so we
             // harden that instruction when we reach it in the instruction
             // sequence.
             if (SunkMI != &MI) {
void X86SpeculativeLoadHardeningPass::hardenLoadAddr(
   if (FlagsReg)
     restoreEFLAGS(MBB, InsertPt, Loc, FlagsReg);
 }
 
 MachineInstr *X86SpeculativeLoadHardeningPass::sinkPostLoadHardenedInst(
     MachineInstr &InitialMI, SmallPtrSetImpl<MachineInstr *> &HardenedInstrs) {
   assert(isDataInvariantLoad(InitialMI) &&
          "Cannot get here with a non-invariant load!");
+  assert(!isEFLAGSDefLive(InitialMI) &&
+         "Cannot get here with a data invariant load "
+         "that interferes with EFLAGS!");
 
   // See if we can sink hardening the loaded value.
   auto SinkCheckToSingleUse =
       [&](MachineInstr &MI) -> Optional<MachineInstr *> {
     Register DefReg = MI.getOperand(0).getReg();
 
     // We need to find a single use which we can sink the check. We can
     // primarily do this because many uses may already end up checked on their
     // own.
     MachineInstr *SingleUseMI = nullptr;
     for (MachineInstr &UseMI : MRI->use_instructions(DefReg)) {
-      // If we're already going to harden this use, it is data invariant and
-      // within our block.
+      // If we're already going to harden this use, it is data invariant, it
+      // does not interfere with EFLAGS, and within our block.
       if (HardenedInstrs.count(&UseMI)) {
-        if (!isDataInvariantLoad(UseMI)) {
+        if (!isDataInvariantLoad(UseMI) || isEFLAGSDefLive(UseMI)) {
           // If we've already decided to harden a non-load, we must have sunk
           // some other post-load hardened instruction to it and it must itself
           // be data-invariant.
           assert(isDataInvariant(UseMI) &&
                  "Data variant instruction being hardened!");
           continue;
         }
 
for (MachineInstr &UseMI : MRI->use_instructions(DefReg)) {
       }
 
       if (SingleUseMI)
         // We already have a single use, this would make two. Bail.
         return {};
 
       // If this single use isn't data invariant, isn't in this block, or has
       // interfering EFLAGS, we can't sink the hardening to it.
-      if (!isDataInvariant(UseMI) || UseMI.getParent() != MI.getParent())
+      if (!isDataInvariant(UseMI) || UseMI.getParent() != MI.getParent() ||
+          isEFLAGSDefLive(UseMI)) {
         return {};
+      }
 
       // If this instruction defines multiple registers bail as we won't harden
       // all of them.
       if (UseMI.getDesc().getNumDefs() > 1)
         return {};
 
       // If this register isn't a virtual register we can't walk uses of sanely,
       // just bail. Also check that its register class is one of the ones we