This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by spatel on Nov 19 2018, 9:32 AM.

Details

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.

Diff Detail

Event Timeline

spatel created this revision.Nov 19 2018, 9:32 AM
spatel edited the summary of this revision. (Show Details)Nov 19 2018, 9:32 AM

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 updated this revision to Diff 175709.Nov 28 2018, 9:37 AM

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 marked an inline comment as done.Nov 28 2018, 9:39 AM
spatel added inline comments.
lib/Target/X86/X86InstrInfo.cpp
2643–2644

Similar bail out?

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 added a comment.Dec 4 2018, 5:07 PM

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.

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?

This revision is now accepted and ready to land.Dec 5 2018, 9:44 AM
This revision was automatically updated to reflect the committed changes.