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.
Details
Diff Detail
Event Timeline
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. |
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 |
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. |
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)!
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. |
Updated DWARFExpression.cpp to support the conditional operators, updated tests to check for them as well
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. |
Updated wrong comment about only handling icmps with one Integer operand, and made handleSSAValueOperands static
As this is function local, can it be static too?