[PATCH] [X86] Special-case 2x CMOV when custom-inserting.

[Matthias Braun]

This approach definitely looks better than the previous CMOV2 attempts.
I think the code should have more comments explaining why the special case for two cmovs is good here. Maybe put in the control flow ascii-art? as the problem is very subtle.

With more comments this LGTM, nitpicks below.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18117
@@ +18116,3 @@
+  MachineInstr *NextCMOV = nullptr;
+  {
+    MachineBasicBlock::iterator InstrIt = MI;
----------------
I'm probably missing something, but it seems unnecessary to me to open a new block.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18118-18119
@@ +18117,4 @@
+  {
+    MachineBasicBlock::iterator InstrIt = MI;
+    ++InstrIt;
+    if (InstrIt != BB->end() && InstrIt->getOpcode() == MI->getOpcode() &&
----------------
MachineBasicBlock::iterator InstrIt = std::next(MI); ? I'd also choose a name like NextMIIt;

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18143-18147
@@ -18118,4 +18142,7 @@
   const TargetRegisterInfo *TRI = Subtarget->getRegisterInfo();
-  if (!MI->killsRegister(X86::EFLAGS) &&
-      !checkAndUpdateEFLAGSKill(MI, BB, TRI)) {
+  auto MIKillsEFLAGS = [BB, TRI](MachineInstr *MI) {
+    return (MI->killsRegister(X86::EFLAGS) ||
+            checkAndUpdateEFLAGSKill(MI, BB, TRI));
+  };
+  if (!MIKillsEFLAGS(NextCMOV ? NextCMOV : MI)) {
     copy0MBB->addLiveIn(X86::EFLAGS);
----------------
Why not simply:
MachineInstr *LastEFlagsUser = NextCMOV ? NextCMOV : MI;
bool EflagsKilled = MI->killsRegister(X86::EFLAGS) || checkAndUpdateEFLAGSKill(LastEFLagsUser, BB, TRI));
if (!EflagsKilled) ...

http://reviews.llvm.org/D8019

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/