[x86] don't try to convert add with undef operands to LEA

[spatel]

Summary

There's existing code here that tries to handle an undef operand while transforming an add to an LEA, but it's incomplete because we will crash on the i16 test with the debug output shown below. I think it's better to just give up instead. Really, GlobalIsel should have folded these before we could get into trouble?

# Machine code for function add_undef_i16: NoPHIs, TracksLiveness, Legalized, RegBankSelected, Selected

bb.0 (%ir-block.0):
  liveins: $edi
  %1:gr32 = COPY killed $edi
  %0:gr16 = COPY %1.sub_16bit:gr32
  %5:gr64_nosp = IMPLICIT_DEF
  %5.sub_16bit:gr64_nosp = COPY %0:gr16
  %6:gr64_nosp = IMPLICIT_DEF
  %6.sub_16bit:gr64_nosp = COPY %2:gr16
  %4:gr32 = LEA64_32r killed %5:gr64_nosp, 1, killed %6:gr64_nosp, 0, $noreg
  %3:gr16 = COPY killed %4.sub_16bit:gr32
  $ax = COPY killed %3:gr16
  RET 0, implicit killed $ax

# End machine code for function add_undef_i16.

*** Bad machine code: Reading virtual register without a def ***
- function:    add_undef_i16
- basic block: %bb.0  (0x7fe6cd83d940)
- instruction: %6.sub_16bit:gr64_nosp = COPY %2:gr16
- operand 1:   %2:gr16
LLVM ERROR: Found 1 machine code errors.

[spatel]

Ping.

[spatel]

Adding more potential reviewers (not sure who knows this layer - suggestions welcome).

Also filed a bug to track the potential global-isel improvement:
https://bugs.llvm.org/show_bug.cgi?id=39827

[spatel]

Patch updated:
Replace the existing code for undef LEA optimization with asserts.
This seems similar to me to how we handle an undef EFLAGS operand at line 2645 of the original file.

[spatel]

lib/Target/X86/X86InstrInfo.cpp

2643–2644

Similar bail out?

[spatel]

Ping * 2.

[aemerson]

This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.

[spatel]

In D54710#1319436, @aemerson wrote:

This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.

Regardless of whether there's a bug in ProcessImplicitDefs, we would have to correctly propagate the undef flag through this transform (from 'add' to 'lea') though? I'm proposing to avoid that for this case based on existing code that appears to do the same thing.

[aemerson]

In D54710#1319529, @spatel wrote:

In D54710#1319436, @aemerson wrote:

This seems to me to be a bug in ProcessImplicitDefs, for some reasoning killing the IMPLICIT_DEF even though there's a subreg use. GlobalISel MIR output looks fine to me.

Regardless of whether there's a bug in ProcessImplicitDefs, we would have to correctly propagate the undef flag through this transform (from 'add' to 'lea') though? I'm proposing to avoid that for this case based on existing code that appears to do the same thing.

Ok, I'm not familiar with the x86 backend to be able to give an opinion on the specifics of this patch. @craig.topper?

[craig.topper]

LGTM

lib/Target/X86/X86InstrInfo.cpp

X86InstrInfo::convertToThreeAddress(MachineFunction::iterator &MFI,
   if (hasLiveCondCodeDef(MI))
     return nullptr;
 
   MachineFunction &MF = *MI.getParent()->getParent();
   // All instructions input are two-addr instructions.  Get the known operands.
   const MachineOperand &Dest = MI.getOperand(0);
   const MachineOperand &Src = MI.getOperand(1);
 
+  // Ideally, operations with undef should be folded before we get here, but we
+  // can't guarantee it. Bail out because optimizing undefs is a waste of time.
+  if (Src.isUndef())
+    return nullptr;
+  if (MI.getNumOperands() > 2)
+    if (MI.getOperand(2).isReg() && MI.getOperand(2).isUndef())
+      return nullptr;
+
   MachineInstr *NewMI = nullptr;
   // FIXME: 16-bit LEA's are really slow on Athlons, but not bad on P4's.  When
   // we have better subtarget support, enable the 16-bit LEA generation here.
   // 16-bit LEA is also slow on Core2.
   bool DisableLEA16 = true;
   bool is64Bit = Subtarget.is64Bit();
 
   unsigned MIOpc = MI.getOpcode();
if (I->getOpcode() == X86::JMP_1) {
       continue;
     }
 
     // Handle conditional branches.
     X86::CondCode BranchCode = X86::getCondFromBranchOpc(I->getOpcode());
     if (BranchCode == X86::COND_INVALID)
       return true;  // Can't handle indirect branch.
 
     // In practice we should never have an undef eflags operand, if we do
     // abort here as we are not prepared to preserve the flag.
     if (I->getOperand(1).isUndef())
       return true;
 
     // Working from the bottom, handle the first conditional branch.
     if (Cond.empty()) {
       MachineBasicBlock *TargetBB = I->getOperand(0).getMBB();
       if (AllowModify && UnCondBrIter != MBB.end() &&
           MBB.isLayoutSuccessor(TargetBB)) {

test/CodeGen/X86/GlobalISel/undef.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=x86_64-linux-gnu            -global-isel -verify-machineinstrs < %s -o - | FileCheck %s --check-prefix=ALL
+; RUN: llc -mtriple=x86_64-linux-gnu -global-isel -verify-machineinstrs < %s -o - | FileCheck %s --check-prefix=ALL
 
 define i8 @test() {
 ; ALL-LABEL: test:
 ; ALL:       # %bb.0:
 ; ALL-NEXT:    retq
   ret i8 undef
 }
 
-define i8 @test2(i8 %a) {
-; ALL-LABEL: test2:
+define i8 @add_undef_i8(i8 %a) {
+; ALL-LABEL: add_undef_i8:
 ; ALL:       # %bb.0:
 ; ALL-NEXT:    movl %edi, %eax
 ; ALL-NEXT:    addb %al, %al
 ; ALL-NEXT:    # kill: def $al killed $al killed $eax
 ; ALL-NEXT:    retq
   %r = add i8 %a, undef
   ret i8 %r
 }
 
+define i16 @add_undef_i16(i16 %a) {
+; ALL-LABEL: add_undef_i16:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %edi, %eax
+; ALL-NEXT:    addw %ax, %ax
+; ALL-NEXT:    # kill: def $ax killed $ax killed $eax
+; ALL-NEXT:    retq
+  %r = add i16 %a, undef
+  ret i16 %r
+}
+
+define i32 @add_undef_i32(i32 %a) {
+; ALL-LABEL: add_undef_i32:
+; ALL:       # %bb.0:
+; ALL-NEXT:    movl %edi, %eax
+; ALL-NEXT:    addl %eax, %eax
+; ALL-NEXT:    retq
+  %r = add i32 %a, undef
+  ret i32 %r
+}
 
 define float @test3() {
 ; ALL-LABEL: test3:
 ; ALL:       # %bb.0:
 ; ALL-NEXT:    retq
   ret float undef
 }