Page MenuHomePhabricator

[DebugInfo] Allow non-stack_value variadic expressions and use in DBG_INSTR_REF
Needs ReviewPublic

Authored by StephenTozer on Sep 15 2022, 3:01 AM.

Details

Reviewers
jmorse
Orlando
Summary

Prior to this patch, variadic DIExpressions (i.e. ones that contain DW_OP_LLVM_arg) could only be created by salvaging debug values to create stack value expressions, resulting in a DBG_VALUE_LIST being created. As of the previous patch in this patch stack, DBG_INSTR_REF's syntax has been changed to match DBG_VALUE_LIST in preparation for supporting variadic expressions. This patch adds some minor changes needed to allow variadic expressions that aren't stack values to exist, and allows variadic expressions that are trivially reduceable to non-variadic expressions to be handled similarly to non-variadic expressions.

Diff Detail

Event Timeline

StephenTozer created this revision.Sep 15 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 3:01 AM
StephenTozer requested review of this revision.Sep 15 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 3:01 AM

Move test changes ahead into this patch.

jmorse added a comment.Oct 6 2022, 5:08 AM

Generally looks good, I've got a couple of questions inline.

Are there any serious performance implications for, in the direction these patches are moving in, there being lots of non-empty DIExpressions compared to today when they're mostly empty? (I'm thinking we might need a static, pre-allocated one in LLVMContext, if every variable is going to allocate one).

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1140–1145

Just to check -- this is purely for printing the informative comments on assembly output, it isn't actually functional, yes?

llvm/lib/IR/DebugInfoMetadata.cpp
1557–1565

Just to confirm, the reason why this wasn't present before was because all variadic expressions had to be stack_value, yes?

llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir
13

Does this patch actually make some DBG_VALUEs become DBG_VALUE_LIST? I've missed that part, and see it's happening in various tests.

jmorse added a comment.Oct 6 2022, 5:09 AM

(Lo and behold, I already asked that last question in D133929, never mind about that)

StephenTozer added inline comments.Oct 6 2022, 5:37 AM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1140–1145

Correct, this is just removing noise from the patch - even if it was an expression being used somewhere, it'd still have the same meaning either way, just formatted slightly differently.

llvm/lib/IR/DebugInfoMetadata.cpp
1557–1565

Correct - worth noting the only reason they "had" to be stack_value was because they're only naturally produced by salvaging operations that create stack_value expressions, not because there's some inherent incompatibility.

Updated ISel and LDV to have very basic handling of "variadic" DBG_INSTR_REFs, and updated tests to use DW_OP_LLVM_arg.

StephenTozer added inline comments.Nov 7 2022, 9:39 AM
llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir
13

Missed this question - this patch changes DBG_INSTR_REF to have the same syntax as a DBG_VALUE_LIST, which includes having the variadic flag set, hence LDV will output the debug value produced from a DBG_INSTR_REF as a DBG_VALUE_LIST.

Move some changes from an earlier patch into this one: Make entry value expressions containing only a leading DW_OP_LLVM_arg, 0 valid, as the changes in this patch enable such expressions. Also add the convertToVariadicExpression function, as this is the first patch in which it is used.