[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

2195

Delete excess parentheses.

[zbrid]

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp

2195

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

X86SpeculativeLoadHardeningPass::tracePredStateThroughIndirectBranches(
 
   // Return all the newly inserted cmov instructions of the predicate state.
   return CMovs;
 }
 
 /// Returns true if the instruction has no behavior (specified or otherwise)
 /// that is based on the value of any of its register operands
 ///
+/// Instructions are considered data invariant even if they set EFLAGS.
+///
 /// A classical example of something that is inherently not data invariant is an
 /// indirect jump -- the destination is loaded into icache based on the bits set
 /// in the jump destination register.
 ///
 /// FIXME: This should become part of our instruction tables.
 static bool isDataInvariant(MachineInstr &MI) {
   switch (MI.getOpcode()) {
   default:
static bool isDataInvariant(MachineInstr &MI) {
   // Arithmetic with just 32-bit and 64-bit variants and no immediates.
   case X86::ADCX32rr: case X86::ADCX64rr:
   case X86::ADOX32rr: case X86::ADOX64rr:
   case X86::ANDN32rr: case X86::ANDN64rr:
   // Unary arithmetic operations.
   case X86::DEC8r: case X86::DEC16r: case X86::DEC32r: case X86::DEC64r:
   case X86::INC8r: case X86::INC16r: case X86::INC32r: case X86::INC64r:
   case X86::NEG8r: case X86::NEG16r: case X86::NEG32r: case X86::NEG64r:
-    // Check whether the EFLAGS implicit-def is dead. We assume that this will
-    // always find the implicit-def because this code should only be reached
-    // for instructions that do in fact implicitly def this.
-    if (!MI.findRegisterDefOperand(X86::EFLAGS)->isDead()) {
-      // If we would clobber EFLAGS that are used, just bail for now.
-      LLVM_DEBUG(dbgs() << "    Unable to harden post-load due to EFLAGS: ";
-                 MI.dump(); dbgs() << "\n");
-      return false;
-    }
-
-    // Otherwise, fallthrough to handle these the same as instructions that
-    // don't set EFLAGS.
-    LLVM_FALLTHROUGH;
 
   // Unlike other arithmetic, NOT doesn't set EFLAGS.
   case X86::NOT8r: case X86::NOT16r: case X86::NOT32r: case X86::NOT64r:
 
   // Various move instructions used to zero or sign extend things. Note that we
   // intentionally don't support the _NOREX variants as we can't handle that
   // register constraint anyways.
   case X86::MOVSX16rr8:

 /// Returns true if the instruction has no behavior (specified or otherwise)
 /// that is based on the value loaded from memory or the value of any
 /// non-address register operands.
 ///
 /// For example, if the latency of the instruction is dependent on the
 /// particular bits set in any of the registers *or* any of the bits loaded from
 /// memory.
 ///
+/// Instructions are considered data invariant even if they set EFLAGS.
+///
 /// A classical example of something that is inherently not data invariant is an
 /// indirect jump -- the destination is loaded into icache based on the bits set
 /// in the jump destination register.
 ///
 /// FIXME: This should become part of our instruction tables.
 static bool isDataInvariantLoad(MachineInstr &MI) {
   switch (MI.getOpcode()) {
   default:
static bool isDataInvariantLoad(MachineInstr &MI) {
   case X86::SUB8rm:
   case X86::SUB16rm:
   case X86::SUB32rm:
   case X86::SUB64rm:
   case X86::XOR8rm:
   case X86::XOR16rm:
   case X86::XOR32rm:
   case X86::XOR64rm:
-    // Check whether the EFLAGS implicit-def is dead. We assume that this will
-    // always find the implicit-def because this code should only be reached
-    // for instructions that do in fact implicitly def this.
-    if (!MI.findRegisterDefOperand(X86::EFLAGS)->isDead()) {
-      // If we would clobber EFLAGS that are used, just bail for now.
-      LLVM_DEBUG(dbgs() << "    Unable to harden post-load due to EFLAGS: ";
-                 MI.dump(); dbgs() << "\n");
-      return false;
-    }
-
-    // Otherwise, fallthrough to handle these the same as instructions that
-    // don't set EFLAGS.
-    LLVM_FALLTHROUGH;
 
   // Integer multiply w/o affecting flags is still believed to be constant
   // time on x86. Called out separately as this is among the most surprising
   // instructions to exhibit that behavior.
   case X86::MULX32rm:
   case X86::MULX64rm:
 
   // Arithmetic instructions that are both constant time and don't set flags.
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