Page MenuHomePhabricator

[DebugInfo] Don't sink DBG_VALUEs past other DBG_VALUEs of the same variable location
ClosedPublic

Authored by jmorse on Nov 25 2019, 8:19 AM.

Details

Summary

This patch is part of a fix for PR43855 [0], resolving problems in D58386 / rGf5e1b718a6 (which ended up being backed out).

It turns out DBG_VALUEs would be sunk even if the variables location was re-specified in the block, so for example in:

 %0 = someinst
DBG_VALUE %0, !123, !DIExpression()
 %1 = anotherinst
DBG_VALUE %1, !123, !DIExpression()

if %0 were to sink, the corresponding DBG_VALUE would sink too, past the next DBG_VALUE, effectively re-ordering assignments.

To fix these things: I've added a SeenDbgVars set recording what variable locations have been seen in a block already (working bottom up), and now flag DBG_VALUEs that would pass a later DBG_VALUE for the same variable.

If such a DBG_VALUE has its operand sunk, then it will now be set to undef, if it can't be copy propagated. I've split out the copy-prop code into a helper function to ease this. This patch is based on a tree with f5e1b718a6 reapplied to make it easier to read.

NB, this only works for repeated DBG_VALUEs in the same basic block, the general case involving control flow is much harder, which I've written up in [1].

[0] https://bugs.llvm.org/show_bug.cgi?id=43855
[1] https://bugs.llvm.org/show_bug.cgi?id=44117

Diff Detail

Event Timeline

jmorse created this revision.Nov 25 2019, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 8:19 AM

Could you please add the motivating example from the description into one of the comments?

Otherwise I think this looks reasonable!

llvm/lib/CodeGen/MachineSink.cpp
117

I have a suspicion that on average, this DenseMap will have very few entries since it contains registers that hold variable values. Would it make sense to use a SmallDenseMap here? Or does it not because the DenseMap is being reused?

jmorse updated this revision to Diff 232133.Dec 4 2019, 7:44 AM

Insert an example into a field comment, to explain the kind of code / DBG_VALUE sequences we want to avoid re-ordering.

jmorse marked an inline comment as done.Dec 4 2019, 7:47 AM
jmorse added inline comments.
llvm/lib/CodeGen/MachineSink.cpp
117

I'd suggest the latter; assuming the concern is memory size, there's only one instance of this field per Pass, so it shouldn't consume too much memory.

(NB, I couldn't immediately find any documentation on SmallDenseMap, so I assume its purpose is memory conservation rather than a performance improvement).

aprantl accepted this revision.Dec 4 2019, 4:16 PM
aprantl added inline comments.
llvm/lib/CodeGen/MachineSink.cpp
117

The reuse argument makes sense to me.

This revision is now accepted and ready to land.Dec 4 2019, 4:16 PM
This revision was automatically updated to reflect the committed changes.