[PATCH] D101970: [X86FixupLEAs] Transform the sequence LEA/SUB to SUB/SUB

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FixupLEAs.cpp:437
+        // register is live.
+        if (!CurInst->findRegisterDefOperand(X86::EFLAGS, /*isDead*/true))
+          return MachineBasicBlock::iterator();
----------------
Can we use MachineInstr::registerDefIsDead


================
Comment at: llvm/lib/Target/X86/X86FixupLEAs.cpp:515
+               .addReg(BaseReg)
+               .addReg(X86::EFLAGS, RegState::ImplicitDefine | RegState::Dead);
+  NewMI2 = BuildMI(MBB, InsertPos, AluI->getDebugLoc(), TII->get(NewOpcode),
----------------
This creates a second EFLAGS implicit def. The BuildMI would have already created one based on the MCInstrDesc. I checked the print-after-all output and saw this on the lea-opt2.ll test1.

```
  $ecx = SUB32rr $ecx(tied-def 0), $edx, implicit-def $eflags, implicit-def dead $eflags                                                                                                                 
  $ecx = SUB32rr $ecx(tied-def 0), $eax, implicit-def $eflags, implicit-def dead $eflags
```

I added -verify-machineinstrs, but it doesn't detect this as an issue. That probably doesn't work well with findRegisterDefOperandIdx.

I think what you want to do is this creating the instruction.

```
NewMI1->addRegisterDead(X86::EFLAGS, TRI);
```

That will scan the operands and mark the existing EFLAGS operand as dead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101970/new/

https://reviews.llvm.org/D101970