Page MenuHomePhabricator

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

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



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

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

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

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.

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.

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