This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Make postra sinking of DBG_VALUEs safe in the presence of subregisters
ClosedPublic

Authored by jmorse on Feb 13 2019, 9:20 AM.

Details

Summary

Currently the machine instruction sinker identifies DBG_VALUE insts that also need to sink by comparing register numbers. Unfortunately this isn't safe, because (after register allocation) a DBG_VALUE may read a register that aliases what's being sunk. To fix this, identify the DBG_VALUEs that need to sink by recording & examining their register units. Register units gives us the following guarantee:

"Two registers overlap if and only if they have a common register unit" [MCRegisterInfo.h]

Thus we can always identify aliasing DBG_VALUEs if the set of register units read by the DBG_VALUE, and the register units of the instruction being sunk, intersect. (MachineSink already uses classes like "LiveRegUnits" for determining sinking validity anyway).

The test added checks for super and subregister DBG_VALUE reads of a sunk copy being sunk as well.

Diff Detail

Event Timeline

jmorse created this revision.Feb 13 2019, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2019, 9:20 AM
aprantl added inline comments.Feb 13 2019, 9:26 AM
lib/CodeGen/MachineSink.cpp
1206

I think you may need to sort here to avoid nondeterministic output when iterating over a Set?

jmorse updated this revision to Diff 186839.Feb 14 2019, 7:03 AM

Sort dbg-values to be sunk to ensure determinism, sprinkle -NEXT on some CHECKS to strengthen test.

jmorse marked 2 inline comments as done.Feb 14 2019, 7:04 AM
jmorse added inline comments.
lib/CodeGen/MachineSink.cpp
1206

Good catch, I've added a sort to avoid relying on the sets order.

jmorse updated this revision to Diff 207580.Jul 2 2019, 9:51 AM
jmorse marked an inline comment as done.

Archaeology-ping: this patch avoids super/sub-register sinking leading to DBG_VALUEs referring to the wrong location, would be good for LLVM-9.

(Update is to add a triple to the command line, I've already tripped up the buildbots enough).

ormris removed a subscriber: ormris.Jul 2 2019, 9:54 AM
vsk added a comment.Jul 10 2019, 1:15 PM

This test looks reasonable. Is D58238 the only MachineSink change you have queued up?

In D58191#1579194, @vsk wrote:

This test looks reasonable. Is D58238 the only MachineSink change you have queued up?

There's also D58386, and it looks like I missed that Adrian put comments on there -- I'll freshen that up shortly.

(Many thanks for giving these patches a look!)

jmorse updated this revision to Diff 209233.Jul 11 2019, 8:36 AM

Facepalm: it looks like I mucked up the diff before uploading, the patch originally came with this change to MachineSink, not just the test. Oooff.

aprantl accepted this revision.Jul 29 2019, 1:09 PM
This revision is now accepted and ready to land.Jul 29 2019, 1:09 PM
vsk added inline comments.Jul 30 2019, 5:13 PM
lib/CodeGen/MachineSink.cpp
1206

Naive question, but: is llvm::sort sorting by address-of-the-MI here, or something else? (Just checking -- if it's the former that's not quite deterministic, and maybe a SetVector is called for)

jmorse updated this revision to Diff 215844.Aug 19 2019, 2:44 AM

/Really/ avoid nondeterminism this time.

jmorse marked an inline comment as done.Aug 19 2019, 2:47 AM
jmorse added inline comments.
lib/CodeGen/MachineSink.cpp
1206

*blinks* *double-takes* Clearly I had a serious caffeine deficiency when writing that code. Updated to use a SetVector as recommended. Single sunk instructions tend to only have a few DBG_VALUEs attached, so I'm not worried about the performance implications.

I'll continue on and land this as I think this qualifies as a trivial update to the patch.

This revision was automatically updated to reflect the committed changes.