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

[Sanjay Patel]

spatel updated this revision to Diff 31241.
spatel added a comment.

Patch updated:

1. Added comment to better explain why the new operands must be dead.
2. Added more asserts to verify that assumptions are intact.
3. Split the EFLAGS check into its own function because eventually we'll want to hoist the non-architecture-specific logic into the MachineCombiner itself (there's already a TODO comment about this).


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
@@ -6437,6 +6437,45 @@
   return false;
 }
 
+/// This is an architecture-specific helper function of reassociateOps. Set the
+/// appropriate flags for any extra operands of instructions that are being
+/// reassociated.
+static void setExtraReassOperands(MachineInstr &OldMI1, MachineInstr &OldMI2,
+                                  MachineInstr &NewMI1, MachineInstr &NewMI2) {
+  // Integer instructions define an implicit EFLAGS source register operand as
+  // the third source (fourth total) operand.
+  if (OldMI1.getNumOperands() != 4 || OldMI2.getNumOperands() != 4)
+    return;
+
+  assert(NewMI1.getNumOperands() == 4 && NewMI2.getNumOperands() == 4 &&
+         "Unexpected instruction type for reassociation");
+  
+  MachineOperand &OldOp1 = OldMI1.getOperand(3);
+  MachineOperand &OldOp2 = OldMI2.getOperand(3);
+  MachineOperand &NewOp1 = NewMI1.getOperand(3);
+  MachineOperand &NewOp2 = NewMI2.getOperand(3);
+
+  assert(OldOp1.isReg() && OldOp1.getReg() == X86::EFLAGS && OldOp1.isDead() &&
+         "Must have dead EFLAGS operand in reassociable instruction");
+  assert(OldOp2.isReg() && OldOp2.getReg() == X86::EFLAGS && OldOp2.isDead() &&
+         "Must have dead EFLAGS operand in reassociable instruction");
+
+  (void)OldOp1;
+  (void)OldOp2;
+
+  assert(NewOp1.isReg() && NewOp1.getReg() == X86::EFLAGS &&
+         "Unexpected operand in reassociable instruction");
+  assert(NewOp2.isReg() && NewOp2.getReg() == X86::EFLAGS &&
+         "Unexpected operand in reassociable instruction");
+
+  // Mark the new EFLAGS operands as dead to be helpful to subsequent iterations
+  // of this pass or other passes. The EFLAGS operands must be dead in these new
+  // instructions because the EFLAGS operands in the original instructions must
+  // be dead in order for reassociation to occur.
+  NewOp1.setIsDead();
+  NewOp2.setIsDead();
+}
+
 /// Attempt the following reassociation to reduce critical path length:
 ///   B = A op X (Prev)
 ///   C = B op Y (Root)
@@ -6503,15 +6542,16 @@
     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.
+  setExtraReassOperands(Root, Prev, *MIB1, *MIB2);
+
+  // 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.31241.patch
Type: text/x-patch
Size: 3925 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150803/3963910c/attachment.bin>