Page MenuHomePhabricator

[DebugInfo] Handle multiple variable location operands in IR
ClosedPublic

Authored by StephenTozer on Sep 24 2020, 8:04 AM.

Details

Summary

Following from the previous patch in this series which enables DIArgLists with a single location operand in debug intrinsics, this patch updates the various IR passes to correctly handle multiple location operands.

Compared to the previous 2 patches, this one has the most actual logic, rather than simply being an interface or data change. This patch does not allow salvageDebugInfo to produce DIArgLists by salvaging instructions with a non-constant second operand; it also does not update almost any of CodeGen. Other than that, it should cover every IR pass.

Most of the changes are relatively straightforward, as we simply extend code that operated on a single debug value to operate on the list of debug values in the style of any_of, all_of, for_each, etc. We also have replaced setOperand(0, ...) with replaceVariableLocationOp, which requires us to pass the variable that is being replaced. In places where this value isn't readily available, we have to track the old value through to the point where it gets replaced.

One of the more complicated cases is insertDebugValuesForPHIs; the comments have been updated to explain the new implementation, but I imagine it'll take some effort to confirm that the implementation does what it says on the tin. Also, a question that I don't have a definitive answer on yet: how do we handle moving dbg.values that depend on multiple instructions? It's quite possible there's no way to preserve many of them, because when we move them to follow a sunk or hoisted instruction we risk losing the other instructions. placeDbgValues simply gives up for dbg.values with multiple location operands, as it isn't particularly reliable even in the single op case. As currently stands, I believe that the new insertDebugValuesForPHIs will always produce valid dbg.values, but am interested to find out if anyone sees an error with the logic or has an example where it fails.

Diff Detail

Event Timeline

StephenTozer created this revision.Sep 24 2020, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 8:04 AM

This mostly LGTM.

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
6

That's how :-)

7

nit: remove redundant {}?

StephenTozer added inline comments.Sep 28 2020, 10:01 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
7

The braces in this case are following the coding standards, specifically following the examples "Use braces for the outer if since the nested for is braced" and "Use braces on the outer block because there are more than two levels of nesting."

mostly lgtm, some initinal comments inline, thanks!

llvm/lib/CodeGen/CodeGenPrepare.cpp
30

Can we add a comment describing what we use this for?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1

So here we will be using the new metadata by salvaging dbg.values with multiple SSA values?

llvm/lib/Transforms/Utils/CodeExtractor.cpp
0

Can we refactor this comments? :)

llvm/lib/Transforms/Utils/Debugify.cpp
3

Should this go into the Verifier, since we do not use this in the SalvageDebugInfo (yet) ?

llvm/lib/Transforms/Utils/LCSSA.cpp
226 ↗(On Diff #294062)

This should be different (nfc) change.

markus added a subscriber: markus.Oct 10 2020, 3:42 AM

Rebase onto recent master, no functional changes from rebase. Minor changes to findDbgUsers/findDbgValues to handle DIArgLists in which the same value appears more than once.

aprantl accepted this revision.Dec 11 2020, 6:42 PM
aprantl added inline comments.
llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
2–1

clang-format?

This revision is now accepted and ready to land.Dec 11 2020, 6:42 PM
StephenTozer added inline comments.Jan 6 2021, 2:08 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1

Yes, this isn't currently implemented in the patch stack, but should be implemented later. It's definitely out of scope for this patch, regardless.

Rebased onto recent master.

This revision was landed with ongoing or failed builds.Mar 9 2021, 8:45 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Mar 17 2021, 5:46 AM

This caused non-deterministic output from the compiler, see https://bugs.chromium.org/p/chromium/issues/detail?id=1189090#c8 for a reproducer. (Usually this is due to non-deterministic iteration order for hash-based data structures such as DenseMap)

I've reverted in 01ac6d1587e8613ba4278786e8341f8b492ac941 until it can be fixed.

This caused non-deterministic output from the compiler, see https://bugs.chromium.org/p/chromium/issues/detail?id=1189090#c8 for a reproducer. (Usually this is due to non-deterministic iteration order for hash-based data structures such as DenseMap)

I've reverted in 01ac6d1587e8613ba4278786e8341f8b492ac941 until it can be fixed.

Resubmitted, should be fixed now - instructions were being inserted while iterating through a map, the map type has been changed from SmallDenseMap to MapVector now, so there should no longer be any non-determinism.