This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Produce variadic DBG_INSTR_REFs from ISel
ClosedPublic

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

Details

Summary

This patch modifies SelectionDAG and FastISel to produce DBG_INSTR_REFs with variadic expressions, and produce DBG_INSTR_REFs for debug values with variadic location expressions. The former essentially means just prepending DW_OP_LLVM_arg, 0 to the existing expression. The latter is achieved in MachineFunction::finalizeDebugInstrRefs and InstrEmitter::EmitDbgInstrRef.

Diff Detail

Event Timeline

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

Generally looks good -- needs tests though.

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
833–835

Haven't these all been excluded due to IsInvalidOp?

jmorse requested changes to this revision.Sep 16 2022, 9:54 AM
This revision now requires changes to proceed.Sep 16 2022, 9:54 AM

The "arg zero" form of DIExpression is going to be a very common case -- is it uniqued, and would it be worth storing a copy somewhere and just using that?

The "arg zero" form of DIExpression is going to be a very common case -- is it uniqued, and would it be worth storing a copy somewhere and just using that?

DIExpressions should be uniqued based on their expression elements, and the get should just be a map lookup underneath, so I don't think there should be any issues with duplicate storage.

Rebased, fixed some errors when emitting undef values and update some tests accordingly.

Added some test coverage for SelectionDAG+FastISel emission of variadic instruction references.

This LGTM with a few inline questions. The addition of DW_OP_arg, 0 to all INSTR_REF expressions does add some visual noise but I personally prefer this. Given how convoluted debug instruction handling can get, I like the idea of continuing in this direction and minimising the number of special cases.

llvm/lib/CodeGen/MachineFunction.cpp
1138

👍

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
698

Possibly worth expanding the comment to mention that these will have been emitted as INSTR_REF by the code above if instr ref is enabled.

856–857

Looks like this one slipped through the cracks - can this TODO be done? IMO eliminating duplicated code is especially important around these fiddly areas.

922

Is there a benefit to preserving the number of operands (rather than just using DBG_VALUE $noreg) - does it strip assertions about DW_OP_arg usage or something if we don't?

StephenTozer added inline comments.Jan 5 2023, 9:20 AM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
922

The only important part about this is that it doesn't invalidate the DIExpression - even for an undef debug value, I believe there are still some points at which having a mismatched number of debug operands with the DIExpression's arg operands is a problem. This could be resolved by also changing the expression, making it empty while any fragment info or other expression elements that apply to undef values - which sounds like something that should be done for all explicitly killed debug values, but happy to start the ball rolling here.

Address comments (extracted MO selection for const debug operands, expanded comments in EmitDbgValue), add a function for setting a DIExpression "undef".

Orlando accepted this revision.Jan 6 2023, 2:07 AM

LGTM

StephenTozer updated this revision to Diff 486868.EditedJan 6 2023, 7:08 AM

Rebased onto latest main, added unit tests for new convertToUndefExpression, and converted some existing tests to test for the new fully-undef debug values that SelectionDAG produces.

Orlando accepted this revision.Jan 6 2023, 7:21 AM

Still LGTM, nice unittest.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 9 2023, 12:58 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.