[PATCH] D11696: [x86] machine combiner reassociation: mark EFLAGS operand as 'dead'

[Sanjay Patel]

spatel created this revision.
spatel added reviewers: Gerolf, kbsmith1, qcolombet.
spatel added a subscriber: llvm-commits.

In the commentary for D11660, I noted that I wasn't sure if it was alright to create new integer machine instructions without also creating the implicit EFLAGS operand. From what I can see, the implicit operand is always created by the MachineInstrBuilder based on the instruction type, so we don't have to do that explicitly. However, in reviewing the debug output, I noticed that the operand was not marked as 'dead'. I think the machine combiner should do that to preserve future optimization opportunities that may be checking for that dead EFLAGS operand themselves.

http://reviews.llvm.org/D11696

Files:
  lib/Target/X86/X86InstrInfo.cpp
  test/CodeGen/X86/machine-combiner-int.ll

Index: test/CodeGen/X86/machine-combiner-int.ll
===================================================================
--- test/CodeGen/X86/machine-combiner-int.ll
+++ test/CodeGen/X86/machine-combiner-int.ll
@@ -1,4 +1,5 @@
-; RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=x86-64 < %s | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=x86-64 | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -print-machineinstrs -o /dev/null 2>&1 | FileCheck %s --check-prefix=DEAD
 
 ; Verify that integer multiplies are reassociated. The first multiply in 
 ; each test should be independent of the result of the preceding add (lea).
@@ -23,6 +24,12 @@
 ; CHECK-NEXT:    imull  %ecx, %edx
 ; CHECK-NEXT:    imull  %edx, %eax
 ; CHECK-NEXT:    retq
+
+; DEAD-LABEL: Machine InstCombiner:
+; DEAD:       ADD32rr
+; DEAD-NEXT:  IMUL32rr{{.*}}%EFLAGS<imp-def,dead>
+; DEAD-NEXT:  IMUL32rr{{.*}}%EFLAGS<imp-def,dead>
+
   %t0 = add i32 %x0, %x1
   %t1 = mul i32 %x2, %t0
   %t2 = mul i32 %x3, %t1
Index: lib/Target/X86/X86InstrInfo.cpp
===================================================================
--- lib/Target/X86/X86InstrInfo.cpp
+++ lib/Target/X86/X86InstrInfo.cpp
@@ -6503,15 +6503,31 @@
     BuildMI(*MF, Prev.getDebugLoc(), TII->get(Opcode), NewVR)
       .addReg(RegX, getKillRegState(KillX))
       .addReg(RegY, getKillRegState(KillY));
-  InsInstrs.push_back(MIB1);
-
   MachineInstrBuilder MIB2 =
     BuildMI(*MF, Root.getDebugLoc(), TII->get(Opcode), RegC)
       .addReg(RegA, getKillRegState(KillA))
       .addReg(NewVR, getKillRegState(true));
-  InsInstrs.push_back(MIB2);
 
-  // Record old instructions for deletion.
+  // Integer instructions define an implicit EFLAGS source register operand.
+  // Mark those operands as dead to be helpful to subsequent iterations of this
+  // pass or other passes.
+  if (MIB1->getNumOperands() == 4) {
+    MachineOperand &Eflags1 = MIB1->getOperand(3);
+    assert(Eflags1.isReg() && Eflags1.getReg() == X86::EFLAGS &&
+           "Unexpected operand in reassociable instruction");
+    Eflags1.setIsDead();
+    
+    assert(MIB2->getNumOperands() == 4 &&
+           "Unexpected instruction type for reassociation");
+    MachineOperand &Eflags2 = MIB2->getOperand(3);
+    assert(Eflags2.isReg() && Eflags2.getReg() == X86::EFLAGS &&
+           "Unexpected operand in reassociable instruction");
+    Eflags2.setIsDead();
+  }
+  
+  // Record new instructions for insertion and old instructions for deletion.
+  InsInstrs.push_back(MIB1);
+  InsInstrs.push_back(MIB2);
   DelInstrs.push_back(&Prev);
   DelInstrs.push_back(&Root);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D11696.31173.patch
Type: text/x-patch
Size: 2635 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150731/37d77782/attachment.bin>