This is an archive of the discontinued LLVM Phabricator instance.

[X86] Avoid codegen changes when DBG_VALUE appears between lowered selects
ClosedPublic

Authored by jmorse on Feb 26 2019, 6:05 AM.

Details

Summary

X86TargetLowering::EmitLoweredSelect currently detects sequences of CMOV pseudo instructions without accounting for debug intrinsics. This leads to different codegen with and without option -g, if a DBG_VALUE instruction lands in the middle of several lowered selects, as demonstrated if you run the added test function below with trunk (additional block branching occurs).

Work around this by skipping over debug instructions when looking for CMOV sequences, and sinking those debug insts into the EmitLoweredSelect sunk block. This might slightly shift where variables appear in the instruction sequence, but won't re-order assignments.

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Feb 26 2019, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2019, 6:05 AM
andreadb added inline comments.Feb 28 2019, 11:04 AM
lib/Target/X86/X86ISelLowering.cpp
28780–28782 ↗(On Diff #188356)

I think you can use function skipDebugInstructionsForward() to update that iterator. It would also make this code a bit more readable.

28821–28829 ↗(On Diff #188356)

Here you iterate from LastCMOV to MI to search for any debug instructions to sink.
Why don't you collect those directly from the loop starting at line 28778.
Basically - if I understand this code correctly - you are scanning the code sequence twice.

andreadb added inline comments.Mar 1 2019, 3:06 AM
lib/Target/X86/X86ISelLowering.cpp
28821–28829 ↗(On Diff #188356)

On a second thought. It may not be easy to do what I suggested in that comment. So, I am okay if you iterate instructions again.
You can probably move this loop before the splice at line 28817. The advantage is that you can perform a potentially more readable forward iteration. It would allow you to can replace calls to insert with calls to SinkMBB->push_back(CurInst).

jmorse updated this revision to Diff 188922.Mar 1 2019, 7:48 AM

Incorporate feedback: use skipDebugInstructionsForward for better readability, iterate forwards when sinking DBG_VALUEs.

(We still have to juggle iterators when sinking the DBG_VALUEs as I believe the current one is invalidated when you call removeFromParent()).

jmorse updated this revision to Diff 188923.Mar 1 2019, 8:53 AM

Incorporate recommendations on using auto from Andrea, delivered offline,

This revision is now accepted and ready to land.Mar 1 2019, 10:15 AM
This revision was automatically updated to reflect the committed changes.