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

[Eric Christopher via Phabricator via llvm-commits]

echristo accepted this revision.
echristo added a comment.
This revision is now accepted and ready to land.

This is obviously good to start and to iterate on.

I started adding some nits and some refactoring comments, but stopped - I think if we break up some of the larger functions it'll be a good start. Can be done later though.

-eric



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:483
+  void splitSuccessor(MachineBasicBlock *Old, MachineBasicBlock *New,
+                      bool NormalizeSuccProbs = false);
+
----------------
You don't currently use the normalization as far as I can tell.


================
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()));
----------------
Can we get a comment on this assert?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.h:327
                      SmallVectorImpl<MachineOperand> &Cond,
-                     bool AllowModify) const override;
+                     bool AllowModify = false) const override;
 
----------------
Not sure why?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:302
+  // implement.
+  if (HardenEdgesWithLFENCE) {
+    SmallSetVector<MachineBasicBlock *, 8> Blocks;
----------------
Can you pull this out into a separate function? Just for readability.


================
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.
----------------
Might want to comment that this is largely a set of optimizations on the lfence insertion?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:317
+        // fallthrough, and set this is the "else" successor.
+        if (!MI.isTerminator() || !MI.isBranch()) {
+          break;
----------------
Braces nit.


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:321
+
+        if (MI.getOpcode() == X86::JMP_1 ||
+            X86::getCondFromBranchOpc(MI.getOpcode()) != X86::COND_INVALID)
----------------
Comment please?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:352
+
+      if (MI.mayLoad() && MI.getOpcode() != X86::MFENCE) {
+        HasCheckableLoad = true;
----------------
Split and continue?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:452
+        int OpIdx = DupIndices.pop_back_val();
+        MI.RemoveOperand(OpIdx + 1);
+        MI.RemoveOperand(OpIdx);
----------------
Go ahead and comment what you're doing here please?


================
Comment at: llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp:1553
+  auto InsertPt = MI.getIterator();
+#if 0
+  if (EFLAGSLive && EFLAGSDefInstr &&
----------------
At least comment why the #if 0 is if'd out?


Repository:
  rL LLVM

https://reviews.llvm.org/D44824