[PATCH] D44824: [Spectre] Introduce a new pass to do speculative load hardening to mitigate Spectre variant #1 for x86.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 17 inline comments as done.
chandlerc added a comment.

Thanks all.

I've made essentially all the suggested changes or nuked the code in question. A bunch of the extra files were just dregs. Not sure why I didn't notice and remove them earlier. They're gone now.

One thing I've left for now is the successor normalization. I explain the motivation, and I'm happy to remove it in a followup if its not convincing.

I'm going to go ahead and land this so that we can start much more actively testing and getting feedback on it. I'm definitely still interested in further comments and will keep working on cleaning some of the issues in this code up. The primary focus I'll have in follow-up patches initially is doing basic refactorings to pull out the different moving pieces here into comprehensible routines with proper comments and such.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:483
+  void splitSuccessor(MachineBasicBlock *Old, MachineBasicBlock *New,
+                      bool NormalizeSuccProbs = false);
+
----------------
echristo wrote:
> You don't currently use the normalization as far as I can tell.
This was to remain consistent with `removeSuccessor`.

I can remove it in a follow-up if you want, I don't really feel strongly here.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:1212
     if (MO.isReg() && MO.isDef()) {
-      assert(MO.isImplicit() && MO.isDead() &&
+      assert(MO.isImplicit() &&
              TargetRegisterInfo::isPhysicalRegister(MO.getReg()));
----------------
echristo wrote:
> Can we get a comment on this assert?
This was working around weird issues I hit using `LAHF` and `SAHF`. I'm no longer using them so I've just dropped this part of the patch.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.h:327
                      SmallVectorImpl<MachineOperand> &Cond,
-                     bool AllowModify) const override;
+                     bool AllowModify = false) const override;
 
----------------
echristo wrote:
> Not sure why?
Sorry, this got copied from some other patch during development. Dropped for now.

But we should actually make some fixes here.

Because we're missing this, we override only *some* of the calls to `analyzeBranch` with the X86-specific logic. =[


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:313
+
+      for (MachineInstr &MI : llvm::reverse(MBB)) {
+        // Once we've handled all the terminators, we're done.
----------------
echristo wrote:
> Might want to comment that this is largely a set of optimizations on the lfence insertion?
Yeah, and it's all just wrong. See below.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:321
+
+        if (MI.getOpcode() == X86::JMP_1 ||
+            X86::getCondFromBranchOpc(MI.getOpcode()) != X86::COND_INVALID)
----------------
echristo wrote:
> Comment please?
I don't know what I was thinking when I wrote this. I've nuked all this code as none of it makes any sense.

Now we just do the much simpler thing of hardening all edges where there were multiple edges coming out of a single block which has branch terminators and where the successors aren't EH pads. Way simpler. Handles way more of the real cases.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:778
+
+  case X86::ADDPDrm:
+  case X86::ADDPSrm:
----------------
craig.topper wrote:
> Can we drop all the vector instructions here? Their AVX and AVX512 equivalents are all missing. It probably makes more sense to add them more methodically. Or add a bit to TSFlags.
Yeah, sorry. I just pasted these as I was going through to find the ones I *wanted* to mark as `true`. But as we aren't handling them, might as well skip for now . Makes it less messy until we can get these into the TD files.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:1123
+  case X86::CVTTSS2SIrm:
+  case X86::CVTTSS2SIrm_Int:
+
----------------
craig.topper wrote:
> AVX versions of these are missing, but we can address as a follow up if you want. Just don't want to lose it since none of the instructions here will be used on Haswell with AVX.
Added a FIXME. This will be worth doing quickly as these actually come up amazingly frequently and so I think its worth handling them, and making sure we do in AVX mode as well.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:1553
+  auto InsertPt = MI.getIterator();
+#if 0
+  if (EFLAGSLive && EFLAGSDefInstr &&
----------------
echristo wrote:
> At least comment why the #if 0 is if'd out?
Sorry, I kept meaning to just delete this and never dit.

I had this great idea of sliding around the position of the hardening instruction to minimize EFLAGS copies. But htat was when EFLAGS copies were basically death to performance. With the new lowering, I'm much less worried and suspect we'll never want to bother with this. It's not so precious of code as we couldn't write it again, and I never debugged it so it is probably full of bad.

Deleted.


Repository:
  rL LLVM

https://reviews.llvm.org/D44824