Page MenuHomePhabricator

[DebugInfo] Handle multiple variable location operands in IR
Needs ReviewPublic

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
1197

That's how :-)

1277

nit: remove redundant {}?

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

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
2768

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

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

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

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

Can we refactor this comments? :)

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

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

llvm/lib/Transforms/Utils/LCSSA.cpp
226

This should be different (nfc) change.

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