This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][4/*] Add llvm.dbg.assign intrinsic boilerplate
ClosedPublic

Authored by Orlando on Aug 19 2022, 6:14 AM.

Details

Summary

The Assignment Tracking debug-info feature is outlined in this RFC. This first series of patches adds documentation, the changes necessary to start emitting and using the new metadata, and updates clang with an option to enable the feature. Working with the new metadata in the middle and back end will come later. There are still a few rough edges but I'm putting these patches up now hoping to get feedback on the design and implementation from the upstream community.


Add the llvm.dbg.assign intrinsic boilerplate. This updates the textual-bitcode roundtrip test to also check that round-tripping with the intrinsic works.

The intrinsic marks the position of a source level assignment.

The llvm.dbg.assign interface looks like this (each parameter is wrapped in MetadataAsValue, and Value * type parameters are
first wrapped in ValueAsMetadata):

void @llvm.dbg.assign(Value *Value,
                      DIExpression *ValueExpression,
                      DILocalVariable *Variable,
                      DIAssignID *ID,
                      Value *Address,
                      DIExpression *AddressExpression)

The first three parameters look and behave like an llvm.dbg.value. ID is a reference to a store. The intrinsic is "linked to" instructions in the same function that use the same ID as an attachment. That is mostly conceptual at this point; the two-way link infrastructure will come in another patch. Address is the destination address of the store and it is modified by AddressExpression. llvm currently encodes variable fragment information in DIExpressions, so as an implementation quirk the FragmentInfo for Variable is contained within ValueExpression only.

Diff Detail

Event Timeline

Orlando created this revision.Aug 19 2022, 6:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 6:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Aug 19 2022, 6:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 6:14 AM
Orlando updated this revision to Diff 456660.Aug 30 2022, 7:14 AM

+ Add context to diff

jmorse added a subscriber: jmorse.Aug 30 2022, 8:25 AM

Looks good with some hand waving in comments; as before, it's good practice to have tests covering the verifier failures.

llvm/include/llvm/IR/IntrinsicInst.h
288–291

This wants three slashes rather than two; I get the feeling the comment is a bit too verbose, in that it's writing a bit too much about one intrinsic in a generic class. IMO, better to focus on "this abstracts where that information is stored".

375

Hmm -- so this is going to mean that feeding a dbg.assign into isa<DbgValueInst>(...) is true, doesn't that risk some misinterpretation out there?

If this is activating some code paths that are necessary for handling / accounting for debug intrinsics, we might want to handle that by putting a new superclass in rather than having dbg.values alias dbg.addrs.

(/me reads further) Ah, because dbg.addr subclasses dbg.value. I can see that there's precedent for this elsewhere (all these things are subclasses of CallInst), I suspect we'll need to think through carefully what the implications are.

llvm/lib/IR/IntrinsicInst.cpp
86

Hmmmm. So this is called _inside_ an assertion, but it also has a side-effect / assignment... doesn't that mean we'll get different behaviour if LLVM is compiled with/without assertions?

94

Missing () on the DbgAssignAddrReplaced call?

179–184

Will this work with DIArgList's? (Probably, figured I'd ask though). Seeing how DbgAddrInst is acting as a DbgValueInst, AFAIUI the loop-strength-reduce and similar code will try and RAUW the value with a DIArgList of values occasionally. I think the semantics of this is obvious, the only risk is that we encode assumptions that the Value is only ever a single ValueAsMetadata.

Thanks for all the reviews. I just wanted to check something (inline) before updating this patch.

llvm/lib/IR/IntrinsicInst.cpp
86

DbgAssignAddrReplaced is a bool assigned the result of the in-line call to the lambda. I can re-arrange this to get rid of the lambda if you'd prefer?

179–184

Ah, sorry, I had intended to remove replaceUsesOfWith and replace calls with replaceVariableLocationOp. I must've missed that change from this patch.

jmorse added inline comments.Sep 6 2022, 8:11 AM
llvm/lib/IR/IntrinsicInst.cpp
86

Aaaaaahhhh cool, I'd missed the trailing parentheses (and "bool" became "auto" in my mind for some reason). I'm not especially bothered about the style, but IMO best to have a dedicated line where the lambda is called, so that someone casually scanning this sees "here's a lambda, here's me calling the lambda". I think that matches the pattern in the rest of llvm.

Orlando updated this revision to Diff 458379.Sep 7 2022, 1:31 AM
Orlando marked 4 inline comments as done.

+ Address feedback
+ Add test for verifier checks

Orlando updated this revision to Diff 458698.Sep 8 2022, 3:36 AM

+ Tidy up comments in test (use ;; rather than ; for non-filecheck-directive comments)
+ Add getRawX versions of DbgAssignIntrinsic methods, following the other intrinsics' examples.
+ Call the getRawX functions in the verifier, rather than using hard-coded getOperand(int) calls.
+ Rename getAssignId -> getAssignID

jmorse accepted this revision.Sep 8 2022, 4:29 AM

LGTM

This revision is now accepted and ready to land.Sep 8 2022, 4:29 AM
This revision was landed with ongoing or failed builds.Nov 7 2022, 2:10 AM
This revision was automatically updated to reflect the committed changes.

(I accidently had an not submitted an inline reply)

llvm/include/llvm/IR/IntrinsicInst.h
375

I did this early on to reduce friction when working on the prototype. I have not tried making DbgAssignIntrinsic a sibling in the inheritance hierarchy and don't have a strong gut feeling for whether the number of clarifying isa<> casts would increase or decrease.