This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fixup LEAs - Fix inconsistent codegen with/without debug info
AcceptedPublic

Authored by cdawson on May 30 2019, 8:32 AM.

Details

Summary

The function processInstrForSlow3OpLEA() contains a register liveness check in isSafeToClobberEFLAGS() which calls computeRegisterLiveness().

computeRegisterLiveness() will not be able to verify that a register is fully dead if the basic block has debug instructions at the start.

This patch fixes computeRegisterLiveness() for this case, which re-enables the optimization.

Diff Detail

Event Timeline

cdawson created this revision.May 30 2019, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 8:32 AM
aprantl added inline comments.May 30 2019, 8:36 AM
llvm/lib/CodeGen/MachineBasicBlock.cpp
1460

Does this skip one instruction more than before unconditionally or am I just misinterpreting the --I?

cdawson updated this revision to Diff 202706.Jun 3 2019, 6:43 AM

Corrected fix to avoid unconditionally moving I backwards by one instruction. Updated computeRegisterLiveness() to ignore meta instructions, not just debug instructions.

cdawson marked an inline comment as done.Jun 3 2019, 6:45 AM
aprantl added inline comments.Jun 3 2019, 8:49 AM
llvm/lib/CodeGen/MachineBasicBlock.cpp
1462

I'm still somewhat confused by this line. Why are you skipping over the instruction *before* I here, but then check I (and not std::prev(I)) in the next line?

aprantl added inline comments.Jun 3 2019, 8:50 AM
llvm/test/CodeGen/X86/fixup-lea-g-no-change.mir
371

Please remove all non-essential attributes. Also, if you can find any way to reduce this testcase, that would also be highly appreciated. It probably doesn't needs to be quite as long.

cdawson marked an inline comment as done.Jun 5 2019, 3:06 AM
cdawson added inline comments.
llvm/lib/CodeGen/MachineBasicBlock.cpp
1462

This is due to the way skipMetaInstructionsBackward() is implemented, it performs a meta check on the provided instruction and decrements backwards if the check succeeds. After we exit the preceding while loop (if we took it) we are either at begin() or a non-meta instruction, so to attempt to skip any earlier potential meta instructions we must look one instr back (which should be alright since we've already checked that we aren't at begin() in the if). skipMetaInstructionsBackward() will either return the first non-meta instruction if finds or begin(), even if begin() is a meta. Since we assign it to I it should still be alright to check I in succeeding if.

Hope that's sufficient info! Let me know if there's anything I didn't explain clearly enough :)

cdawson marked an inline comment as done.Jun 5 2019, 3:11 AM
cdawson added inline comments.
llvm/test/CodeGen/X86/fixup-lea-g-no-change.mir
371

How would you recommend trying to reduce this test case?

ormris removed a subscriber: ormris.Jun 5 2019, 9:18 AM
aprantl accepted this revision.Jun 7 2019, 2:02 PM
This revision is now accepted and ready to land.Jun 7 2019, 2:02 PM
arsenm resigned from this revision.Apr 5 2020, 6:52 AM

Is this still needed?

Ping. This still does look relevant. Anyone feels like committing this?

I think this patch needs to be rebased. There have been changes in MachineBasicBlock::computeRegisterLiveness