This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Correctly sink DBG_VALUEs in postra-machine-sink
ClosedPublic

Authored by jmorse on Nov 1 2018, 12:44 PM.

Details

Summary

As reported in PR38952 [0] the postRA machine-code sinking of copies relies on relevant DBG_VALUEs always immediately following the instruction that's being sunk. The PR has a counterexample to this, and DBG_VALUE location does not seem guaranteed after the register allocator runs.

Because postra-machine-sink walks backwards through each BB looking for copies to sink, it will walk past any DBG_VALUE that refers to a copy it subsequently sinks. This means we don't need to scan the whole block looking for DBG_VALUEs, only record which ones have already been seen.

This patch collects seen DBG_VALUEs into a PhysRegister : DBG_VALUE multimap, and hands a vector of relevant DBG_VALUEs down to performSink if a copy gets sunk. Multimap is expensive, but there are no guarantees AFAIK about what order we'll encounter DBG_VALUEs in or what registers they may refer to, so I can't see what else to use.

Highly relevant to this patch is the discussion in [1] regarding the validity of sinking DBG_VALUEs past other DBG_VALUEs of the same variable, re-ordering the appearance of values for the user. My summary would be "yes that's a problem, but it's currently an acceptable tradeoff" (YMMV).

Building debug clang+llvm using another clang+llvm with and without this patch, completes in the same amount of time (give or take a second).

[0] https://bugs.llvm.org/show_bug.cgi?id=38952
[1] https://reviews.llvm.org/D45637

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Nov 1 2018, 12:44 PM

Because it's not completely clear in the summary: the reliance on DBG_VALUEs following the insn being sunk is caused by performSink's use of MachineInstr::collectDebugValues

Presumably this addresses the bad debugging experience for the sample from the PR; does it have any other good/bad effect on the Dexter corpus?

test/CodeGen/X86/pr38952.mir
72 ↗(On Diff #172198)

The way this comment is written looks like an alternate CHECK line, except it isn't. You can just say "Test that the DBG_VALUE ..." and avoid the potential confusion.

Please review all comments to ensure proper punctuation. There are a lot of missing full-stops.

lib/CodeGen/MachineSink.cpp
741 ↗(On Diff #172198)

Comment needs to finish the thought; or is it just missing a full-stop?

1182 ↗(On Diff #172198)

Did clang-format-diff let you do this? Normally 'continue' would be on the next line.

aprantl added inline comments.Nov 1 2018, 1:21 PM
lib/CodeGen/MachineSink.cpp
960 ↗(On Diff #172198)

Would a (Small)DenseMap<SmallVector> be more efficient?

1129 ↗(On Diff #172198)

Nitpick: Please always use full sentences in comments with a trailing .

rnk added a subscriber: rnk.Nov 1 2018, 1:53 PM
rnk added inline comments.
lib/CodeGen/MachineSink.cpp
960 ↗(On Diff #172198)

Peanut gallery: DenseMap of Small* things are usually not efficient. There is TinyPtrVector, though, which is optimized for this use case, so DenseMap<unsigned, TinyPtrVector<MachineInstr*>> is probably a good choice.

mattd added inline comments.Nov 1 2018, 2:07 PM
lib/CodeGen/MachineSink.cpp
1116 ↗(On Diff #172198)

nit: unnecessary newline at L1116

1130 ↗(On Diff #172198)

Can you get away with using MI->isDebugInstr ?

1140 ↗(On Diff #172198)

As an alternative to insert, you can call emplace().
SeenDbgInstrs.emplace(MO.getReg(), MI);

1145 ↗(On Diff #172198)

This condition might not be necessary since you will always continue in the previous conditional at L1130... if you update the conditional at L1130 to be isDebugInstr.

jmorse updated this revision to Diff 172362.Nov 2 2018, 8:11 AM

Rewrite comments, use DenseMap<u, TinyPtrVector> instead of multimap

jmorse marked 9 inline comments as done.Nov 2 2018, 8:34 AM

Hi,

Presumably this addresses the bad debugging experience for the sample from the PR; does it have any other good/bad effect on the Dexter corpus?

Alas there's no other effect on the rest of the tests, only the original [0], although we also get the correct lifetime for the function argument now. (Previously it was "optimized out" everywhere after the start of the function).

[0] https://github.com/jmorse/dexter/blob/dextests/tests/nostdlib/llvm_passes/Vectorize/MissingArgVal/missingargval.cpp

lib/CodeGen/MachineSink.cpp
960 ↗(On Diff #172198)

Now using the suggested container; running a Debug clang/llvm build again gave one faster time, one slower time, both within the margin of error. It looks like this container form will match the common case very well, and in the worst case be no worse than multimap.

1130 ↗(On Diff #172198)

I don't believe so -- that matches DebugLabels as well which don't get sunk, so there'd need to be another condition filtering them out.

(NB, at this time I don't really know what DebugLabels are used for, so this opinion isn't based on any knowledge).

1140 ↗(On Diff #172198)

(Container changed to one that doesn't have emplace).

aprantl accepted this revision.Nov 2 2018, 8:46 AM

one more documentation request inline.

lib/CodeGen/MachineSink.cpp
737 ↗(On Diff #172362)

Please document what the optional DbgVals parameter is for. The next time someone reads this function they will be very confused about it :-)

This revision is now accepted and ready to land.Nov 2 2018, 8:46 AM
This revision was automatically updated to reflect the committed changes.
jmorse marked 2 inline comments as done.