[PATCH] D45146: [x86] Introduce a pass to begin more systematically fixing PR36028 and similar issues.

Mon Apr 2 11:11:22 PDT 2018

rnk added a comment.

\o/

================
Comment at: llvm/lib/Target/X86/X86.h:73

+/// Return a pass that lowers EFLAGS copy nodes.
+FunctionPass *createX86FlagsCopyLoweringPass();
----------------
nit: They're really pseudo instrutions. Now that we're not in the DAG, calling them nodes doesn't seem accurate.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:11-14
+/// Lowers COPY nodes of EFLAGS to directly extract and preserve individual
+/// flag bits so that we don't need to use pushf/popf which have serious
+/// problems due to IF and other flags that should *not* be preserved across
+/// arbitrary instructions in all cases. In addition to these correctness
----------------
I feel like the first one sentence summary of why we need this pass shouldn't include "so we don't need pushf/popf". That presumes the reader already thinks that "popf" is a solution the problem, when it was never actually a viable solution to begin with for all the reasons you outline. The first sentence should state directly that there is no correct or efficient way to copy comparison flags on x86. Then you can go on and explain that the reader might consider popf, lahf, etc, but these are innefficient and incorrect.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:101
+  /// our predicate state through the exiting edges.
+  struct BlockCondInfo {
+    MachineBasicBlock *MBB;
----------------
This looks dead, I can't find it in the rest of the pass.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:357
+    if (DOp.isDead())
+      // Nothing to do here.
+      continue;
----------------
Any reason not to delete dead copies so we have the invariant that after this pass, regardless of future pass ordering, no flags copies will be emitted?

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:371
+    // jumps because their usage is very constrained.
+    bool IsKilled = false;
+    SmallVector<MachineInstr *, 4> JmpIs;
----------------
Maybe call it `FlagsKilled`.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:399-401
+      // Once we encounter a branch, we have a predictable scan to and we can
+      // quickly batch them up. We also can't rewrite in place here and
+      // handle that below.
----------------
This would be clearer as:
Once we encounter a branch, the rest of the instructions must also be branches. We can't rewrite them in place here, so we handle them below.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:403
+      //
+      // FIXME: handle conditional tail calls
+      if (X86::getCondFromBranchOpc(MI.getOpcode()) != X86::COND_INVALID) {
----------------
This might need to be addressed before committing.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:424
+        // We assume that instructions that use flags also def them.
+        assert(MI.findRegisterDefOperand(X86::EFLAGS) &&
+               "Expected a def of EFLAGS for this instruction!");
----------------
Would it be better to assert this and set IsKilled before the rewrite? This is similar to how you set the IsKilled bool above before rewrites.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:449-454
+      DEBUG(
+          dbgs() << "ERROR: Found a copied EFLAGS live-out from basic block:\n"
+                 << "----\n";
+          MBB.dump(); dbgs() << "----\n"
+                             << "ERROR: Cannot lower this EFLAGS copy without "
+                                "using dangerous popf construct!\n");
----------------
Do you want to commit this? If so, I'd add braces (`DEBUG({ ... });`) and re-format it.

Given that we report_fatal_error here, it doesn't seem like we need to mention popf in the logs. The plan is to remove that physreg copy logic so LLVM never emits it.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:462-464
+      MachineBasicBlock &JmpMBB =
+          JmpI == JmpIs.front() ? MBB
+                                : splitBlock(*JmpI->getParent(), *JmpI, *TII);
----------------
`splitBlock` has lots of side effects. This would be clearer as:
  MachineBasicBlock *JmpMBB = &MBB;
  if (JmpI != JmpIs.front())
    JmpMBB = &splitBlock(...);

It's used as a pointer in the assert anyway.

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:470-471
+
+    // FIXME: Mark the last use of EFLAGS before the copy's def as a kill if
+    // the copy's def operand is itself a kill.
+
----------------
I see. Yeah, that is annoying to implement. Don't later passes try to infer this logic? Maybe it's OK to rely on that?

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:479-480
+
+  DEBUG(dbgs() << "\nFunction after rewriting copies:\n"; MF.dump();
+        dbgs() << "\n"; MF.verify(this));
+  return true;
----------------
This seems redundant with -print-after=

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:561
+
+  switch (getMnemonicFromOpcode(MI.getOpcode())) {
+  case FlagArithMnemonic::ADC:
----------------
This seems like the only use of getMnemonicFromOpcode. It seems like you could simplify all that stuff down to just three states:
1. non-flags-using arithmetic
2. arithmetic using CF
3. arithmetic using OF (ADDOX)

I'm guessing this more detailed classification of opcodes is useful if we want to do sophisticated ADDOX optimizations for multi-precision arithmetic. Do you think it's worth keeping around?

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:584-585
+
+  // Insert an instruction that will set the flag back to the desired value.
+  unsigned TmpReg = MRI->createVirtualRegister(PromoteRC);
+  auto AddI =
----------------
Hah, I was scratching my head at this, but I guess at this point during codegen we haven't done two-operand conversions yet. :)

================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:620
+INITIALIZE_PASS_BEGIN(X86FlagsCopyLoweringPass, DEBUG_TYPE,
+                      "X86 speculative load hardener", false, false)
+INITIALIZE_PASS_END(X86FlagsCopyLoweringPass, DEBUG_TYPE,
----------------
Pass name is wrong. Blech, boiler-plate. =P

Repository:
  rL LLVM

https://reviews.llvm.org/D45146