This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][DebugInfo] salvageDebugInfo analogue for gMIR
ClosedPublic

Authored by dzhidzhoev on Jul 15 2022, 3:42 PM.

Details

Summary

Salvage debug info of instruction that is about to be deleted as dead in Combiner pass. Currently supported instructions are COPY and G_TRUNC.

It allows to salvage debug info of some dead arguments of functions, by putting DWARF expression corresponding to the instruction being deleted into related DBG_VALUE instruction.

Here is an example of missing variables location https://godbolt.org/z/K48osb9dK. We see that arguments x, y of function foo are not available in debugger, and corresponding DBG_VALUE instructions have undefined register operand instead of variables locaton after Aarch64PreLegalizerCombiner pass. The reason is that registers where variables are located are removed as dead (with instruction G_TRUNC). We can use salvageDebugInfo analogue for gMIR to preserve debug locations of dead variables.

Statistics of llvm object files built with vs without this commit on -O2 optimization level (CMAKE_BUILD_TYPE=RelWithDebInfo, -fglobal-isel) on Aarch64 (macOS):

Number of variables with 100% of parent scope covered by DW_AT_location has been increased by 7,9%.
Number of variables with 0% coverage of parent scope has been decreased by 1,2%.
Number of variables processed by location statistics has been increased by 2,9%.
Average PC ranges coverage has been increased by 1,8 percentage points.

Coverage can be improved by supporting more instructions, or by calling salvageDebugInfo for instructions that are deleted during Combiner rules exection.

Diff Detail

Event Timeline

dzhidzhoev created this revision.Jul 15 2022, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 3:42 PM
dzhidzhoev requested review of this revision.Jul 15 2022, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 3:42 PM
dzhidzhoev edited the summary of this revision. (Show Details)Jul 15 2022, 3:48 PM

Fix X86 test

dzhidzhoev added a comment.EditedJul 19 2022, 4:20 PM

Statistics of llvm object files built with vs without this commit on -O2 optimization level (CMAKE_BUILD_TYPE=RelWithDebInfo, -fglobal-isel) on Aarch64 (macOS):

Number of variables with 100% coverage of parent scope has been increased by 7,9%.
Number of variables with 0% coverage of parent scope has been decreased by 1,2%.
Number of variables processed by location statistics has been increased by 2,9%.
Average PC ranges coverage has been increased by 1,8 percentage points.

Ignore partially formed DBG_VALUEs being used only for testing

dzhidzhoev edited the summary of this revision. (Show Details)Jul 20 2022, 5:56 AM
dzhidzhoev added reviewers: probinson, jmorse.

This seems conceptually fine to me -- it's applying an existing pattern (of salvaging) in GISel where there's coverage loss. I don't feel confident enough to give a proper review though as I've never touched GISel. I would suggest though that you'll need some special handling for the DBG_VALUE "IsIndirect" flag (operand 1): when set to an integer rather than $noreg, it causes an implicit DW_OP_deref to be added at the end of compilation. If you convert the DIExpression to contain a DW_OP_stack_value, that doesn't work.

I feel obliged to point out that the "instruction referencing" re-design of variable locations on x86 [0] avoids problems like this by never referring to copies, so it never has to deal with problems like this. Implementing that for other architectures is work that I'm not able to do, alas.

[0] https://lists.llvm.org/pipermail/llvm-dev/2021-November/153653.html

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

Ignore indirect DBG_VALUE instructions, addressing @jmorse comment.

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

salvageDebugInfoForDbgValue is not specific to GlobalISel in fact. It could be accessible for other passes, with support of other unary/binary instructions.

Nice! Is the primary implementation of salvageDebugInfoForDbgValue specific to GlobalISel, or should this be a general MIR utility that is accessible from all MIR passes?

salvageDebugInfoForDbgValue is not specific to GlobalISel in fact. It could be accessible for other passes, with support of other unary/binary instructions.

In that case would recommend to move it into, e.g., lib/CodeGen/MachineInstr.cpp so it is clearly marked as a generic utility that encourages integration in other passes.

Rebasing & addressing @aprantl comment. Thanks!

Fix build fail

Fix build fail #2

Move salvageDebugInfo to CodeGenCommonISel.cpp, since it could be used from GlobalISel and other codegens

Fix LLVM_DEBUG error

aprantl accepted this revision.Jul 29 2022, 3:14 PM

I probably would have moved it even outside of CodeGenCommonIsel into the something that is included by all of CodeGen, since it's not ISel-specific and other passes might also delete instructions, but this is fine.

llvm/test/CodeGen/AArch64/GlobalISel/salvage-debug-info-dead.mir
3

Please comment what's being tested here.

This revision is now accepted and ready to land.Jul 29 2022, 3:14 PM

Addressing @aprantl comment.

dzhidzhoev marked an inline comment as done.Jul 29 2022, 4:59 PM

Please, commit this for me, because I have no commit access.
Vladislav Dzhidzhoev <vdzhidzhoev@accesssoftek.com>

This revision was landed with ongoing or failed builds.Aug 1 2022, 2:15 AM
This revision was automatically updated to reflect the committed changes.