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

[Gerolf Hoflehner]

Why would reassociation result in dead eflags again? Because it can kick-in only when eflags are dead before reassoc, so they must be dead after? The comment should be clear about why setting the attribute is correct.

-Gerolf

> On Jul 31, 2015, at 3:28 PM, Sanjay Patel <spatel at rotateright.com> wrote:
> 
> 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);
> }
> 
> 
> <D11696.31173.patch>