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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:545-546
+    X86::CondCode Cond = X86::getCondFromSETOpc(MI.getOpcode());
+    if (Cond != X86::COND_INVALID && MI.getOperand(0).isReg())
+      CondRegs[Cond] = MI.getOperand(0).getReg();
+
----------------
Do you think we need to check if this is a virtual register? It won't work if it's a physreg, but I also can't imagine how we'd end up with a SETcc into a physical register...


================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:548
+
+    // If we hit an instruction that defines EFLAGS we can stop scanning.
+    if (MI.findRegisterDefOperand(X86::EFLAGS))
----------------
It's a correctness issue to keep scanning, since if you find other SETcc's, they will be from the wrong flag state. This might be clearer as: Stop scanning when we find the instruction that defined the flags.


================
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
----------------
chandlerc wrote:
> rnk wrote:
> > 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.
> Yeah, totally rewrote this. Lemme know what you think.
Looks good


================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:561
+
+  switch (getMnemonicFromOpcode(MI.getOpcode())) {
+  case FlagArithMnemonic::ADC:
----------------
chandlerc wrote:
> rnk wrote:
> > 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?
> I can simplify it if you really want.
> 
> Honestly, having done the simplification, it's a mess to read. The issue is that the macros to do all the silly matching of the crazy number of instruction op codes ends up making it really hard to read the business logic here. So I think just for readability, I'd rather keep the enum and static mapping function.
> 
> The use of a switch here is just to force us to update it when/if we add a new mnemonic.
Makes sense.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:6804
+    // found. This path should be removed after the LLVM 7 release.
+    report_fatal_error("Unable to copy EFLAGS physical register!");
   }
----------------
\o/


================
Comment at: llvm/test/CodeGen/X86/cmpxchg-clobber-flags.ll:11-12
 
 ; TODO: Reenable verify-machineinstr once the if (!AXDead) // FIXME
 ; in X86InstrInfo::copyPhysReg() is resolved.
 
----------------
rnk wrote:
> These comments are super stale. Maybe update them to reflect the new checks?
Can we do this now?


================
Comment at: llvm/test/CodeGen/X86/cmpxchg-clobber-flags.ll:11-29
 ; TODO: Reenable verify-machineinstr once the if (!AXDead) // FIXME
 ; in X86InstrInfo::copyPhysReg() is resolved.
 
 declare i32 @foo()
 declare i32 @bar(i64)
 
 ; In the following case when using fast scheduling we get a long chain of
----------------
These comments are super stale. Maybe update them to reflect the new checks?


================
Comment at: llvm/test/CodeGen/X86/flags-copy-lowering.mir:10-11
+  
+  define i32 @test_branch(i64 %a, i64 %b) {
+  entry:
+    call void @foo()
----------------
I guess all this IR up here is MIR boilerplate. =/


================
Comment at: llvm/test/CodeGen/X86/mul-i1024.ll:1
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i386-unknown | FileCheck %s --check-prefix=X32
----------------
This seems like a questionable use of auto-generated llc checks. :(


================
Comment at: llvm/test/CodeGen/X86/peephole-na-phys-copy-folding.ll:5-6
 
 ; TODO: Reenable verify-machineinstrs once the if (!AXDead) // FIXME in
 ; X86InstrInfo::copyPhysReg() is resolved.
 
----------------
Can we do this?


Repository:
  rL LLVM

https://reviews.llvm.org/D45146