This is an archive of the discontinued LLVM Phabricator instance.

Add support for salvaging debug info from icmp instrcuctions.
ClosedPublic

Authored by rastogishubham on May 9 2023, 11:34 AM.

Details

Summary

salvageDebugInfo() is a function that allows us to reatin debug info for instructions that have been optimized out. Currently, it doesn't support salvaging the debug information from icmp instrcutions, but DWARF expressions can emulate an icmp by using the DWARF conditional expressions. This patch adds support for salvaging debug information from icmp instructions.

Diff Detail

Event Timeline

rastogishubham created this revision.May 9 2023, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 11:34 AM
Herald added subscribers: hoy, hiraditya. · View Herald Transcript
rastogishubham requested review of this revision.May 9 2023, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 11:34 AM
bulbazord added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
2056

nit: you can drop the check for ConstInt in this condition because it must be valid due to the above check.

2064

nit: You could push_back(ConstInt) since it's the same thing you already computed instead of calling Icmp->getOperand(1) again.

2068–2070

You've already changed AdditionalValues by the time you get to this point... Do the callers verify that the return value of this function is nullptr before using AdditionalValues? Even if they do today, somebody may use this function without realizing that this is the behavior with surprising results. My suggestion would be to only mutate the output references (Opcodes and AdditionalValues) when you know that this function will succeed.

rastogishubham marked 3 inline comments as done.

Removed unnecessary check for ConstInt, removed redundant call to getOperand(1) and moved check for predicate above mutating AdditionalValues

llvm/lib/Transforms/Utils/Local.cpp
2068–2070

That is a good catch, the check for the predicate doesn't have to be done at the end, so I can move it above the mutation of AdditionalValues

aprantl added inline comments.May 10 2023, 10:27 AM
llvm/lib/Transforms/Utils/Local.cpp
1996

I guess the assumption here is that in the typed stack the signedness of the operation is implicit? Might be good to document that here.

llvm/test/DebugInfo/salvage-icmp.ll
16

Can you add tests for the other operations, too?

rastogishubham marked 2 inline comments as done.

Updated test to check all operations, changed output to emit a DW_OP_constu or DW_OP_consts instead of a DW_OP_llvm_arg for constant values

A few comments and questions inline - glad to see more operators being added to the salvageable list!

llvm/lib/Transforms/Utils/Local.cpp
2049–2051

Is there a reason for the IntType template here? At the moment we wouldn't expect any type other than uint64_t to appear here, as it's the width of a DWARF expression operand.

2055–2057

I don't think this early-exit to prevent a large DIExpression should be necessary - the only functions that call into salvageDebugInfoImpl either don't accept any additional values, or have a limit on the size of the DIExpression they will produce.

2062–2066

I think this is the cause of the (as far as I can tell) erroneous DW_OP_LLVM_args in the expression in the test; these steps should only happen if you're salvaging a variadic expression, i.e. if Icmp->getOperand(1) is an Instruction rather than a ConstantInt - it should be fine if you just delete these lines outright, unless you change the function to also support salvaging a pair of Instructions.

To summarize the intention of this code, DW_OP_LLVM_arg should only appear in variadic expressions as a way to represent the list of SSA values being passed to the expression. The first four lines check whether the expression is already variadic and converts it to be variadic if not, and then the last line adds the LLVM_arg for the new SSA value (which in this case does not exist), while the SSA value is added to AdditionalValues. The existing functions are not commented or documented very well, my apologies!

llvm/test/DebugInfo/salvage-icmp.ll
7–9

Does this test pass as written? I see references to various values of DW_OP_LLVM_arg in the expression, but the !DIArgList contains 1 element - I'd expect this to trigger an assertion.

rastogishubham marked 2 inline comments as done.

Added support for non-constant values, fixed bug with DW_OP_llvm_arg, added a new testcase to check for non-constant values

llvm/lib/Transforms/Utils/Local.cpp
2049–2051

I am sorry about that, I was working on a patch where I was going to pass an int64_t for signed comparisons, I ended up realizing that a DW_OP_consts can be used with a uint64_t just fine, so I removed the template everywhere, but I guess I missed a place

2062–2066

Thanks for explaining the logic to me, I did get a bit confused there

NFC change, moved common code in handling binary instructions and icmp instructions with both operands as ssa values into its own function

Thanks, I think all of my concerns have been addressed!
Does lib/CodeGen/AsmPrinter/DwarfExpression.cpp need to be updated for the new opcodes or will just work?

Thanks for the update, changes look good (with some minor comments inline)!

Thanks, I think all of my concerns have been addressed!
Does lib/CodeGen/AsmPrinter/DwarfExpression.cpp need to be updated for the new opcodes or will just work?

That's a good point - currently these ops will be unhandled and trigger an assert in DwarfExpression::addExpression; after fixing that up, it'd probably be a good idea to add one or more extra tests to exercise ISel and DWARF emission. You could model the tests off of llvm\test\DebugInfo\X86\debug_value_list_selectiondag.ll and llvm\test\DebugInfo\X86\dbg_value_list_emission.mir, or add additional debug values to existing tests to cover them.

llvm/lib/Transforms/Utils/Local.cpp
2018–2019

Very small point, but the coding standards recommend braces to be used for else if the corresponding if is using them.

2072–2073

Ditto the above comment.

rastogishubham marked 4 inline comments as done.

Added braces for the else, since the if also has them

Updated DWARFExpression.cpp to support the conditional operators, updated tests to check for them as well

Added new line at the end of debug_value_list_selectiondag.ll

This sounds like a good direction to take; a comment inline about whether the patch as-is only salvages icmps with one constant operand (it appears to salvage more?).

It might be worth relaxing that condition and allowing all icmps to be salvaged, if it doesn't significantly increase file size on some large code samples.

llvm/lib/Transforms/Utils/Local.cpp
1985

As this is function local, can it be static too?

2058–2059

Possibly I've missed something here, but this statement (only salvage icmps with a constant integer operand) doesn't seem to be true, as the second dbg.value in the test you're adding salvages many icmps with Value operands:

%icmp18 = icmp slt i32 %icmp17.1, %b, !dbg !15
%icmp19 = icmp ule i32 %icmp18.1, %a, !dbg !15
%icmp20 = icmp sle i32 %icmp19.1, %a, !dbg !15

and the corresponding CHECK line has a lot Value inputs in the DIArgList.

rastogishubham marked 2 inline comments as done.

Updated wrong comment about only handling icmps with one Integer operand, and made handleSSAValueOperands static

aprantl accepted this revision.May 23 2023, 11:32 AM
This revision is now accepted and ready to land.May 23 2023, 11:32 AM
This revision was landed with ongoing or failed builds.May 23 2023, 3:31 PM
This revision was automatically updated to reflect the committed changes.