This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][21/*] Account for assignment tracking in inliner
ClosedPublic

Authored by Orlando on Sep 5 2022, 8:48 AM.

Details

Summary

The inliner requires two additions:

fixupAssignments - Update inlined instructions' DIAssignID metadata so that inlined DIAssignID attachments are unique to the inlined instance.

trackInlinedStores - Treat inlined stores to caller-local variables (i.e. callee stores to argument pointers that point to the caller's allocas) as assignments. Track them using trackAssignments, which is the same method as is used by the AssignmentTrackingPass. This means that we're able to detect stale memory locations due to DSE after inlining. Because the stores are only tracked _after_ inlining, any DSE or movement of stores _before_ inlining will not be accounted for. This is an accepted limitation mentioned in the RFC.

One change is also required:

Update CloneBlock to preserve debug use-before-defs. Otherwise the assignments will be dropped due to having the intrinsic operands replaced with empty metadata (see use-before-def.ll in this patch and this related discourse post.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:48 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Sep 5 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:48 AM
jmorse added a subscriber: jmorse.Sep 12 2022, 9:12 AM

Looks good, with the caveat that I think you need to explicitly check for DIAssignID nodes in the output of tests to make FileCheck notice if something is merged / distinct (see inline comments).

llvm/lib/Transforms/Utils/CloneFunction.cpp
385–389

I'd suggest not mentioning assignment tracking at all here, that won't be a consideration of the "average" reader. IMO it should be alright to curtly mention that debug intrinsics are handled later, and shift any commentary on use-before-def to wherever the debug intrinsics are maintained. (This is purely for the purpose of not burdening the reader with too much information).

719–720

Just so that I understand, vmap is a mapping from the old function to the duplicated instructions in the new -- so we're only remapping those debug intrinsics that survived the inlining?

(This seems fine to me).

llvm/lib/Transforms/Utils/InlineFunction.cpp
1564

I feel the reader will be happier if there's a typedef for this return type (visually easier to parse too).

1630

It feels like a slightly abnormal usage that Map.lookup is going to default-allocate the NewID as nullptr, so lookup also initializes. AFAIUI there's nothing wrong with this though.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/id.ll
4–6

The inlined (_Z3funv) function is in the output as well -- IMO there's value in testing that it's distinct from the other IDs.

Also, unless you check at the end for the existence of two DIAssignID nodes with dedicated numbers, I think ID_0 and ID_1 can end up having the same value, and this isn't an error as far as FileCheck is concerned.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll
176

As there are two variables !88 !89 in this function, shouldn't we check for both variables in the CHECK lines?

215

As with earlier, you'll need to check for DIAssignID nodes to ensure that none are merged / duplicated accidentally.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/use-before-def.ll
20–23

SGTM -- having a well defined meaning for this kind of scenario is exactly the outcome we should see from treating assignments as assignments in the compiler.

jmorse requested changes to this revision.Sep 13 2022, 9:49 AM

(Sending back to "revise" queue given review above)

This revision now requires changes to proceed.Sep 13 2022, 9:49 AM
Orlando updated this revision to Diff 460340.Sep 15 2022, 3:01 AM
Orlando marked 6 inline comments as done.

+ Address review comments
+ Remove tbaa from tests

Should I move the preservation of use-before-defs to separate patch since it affects dbg.values too?

llvm/lib/Transforms/Utils/CloneFunction.cpp
385–389

Is this OK?

719–720

Just so that I understand, vmap is a mapping from the old function to the duplicated instructions in the new

I believe so.

so we're only remapping those debug intrinsics that survived the inlining?

What do you mean by survived the inlining?

llvm/lib/Transforms/Utils/InlineFunction.cpp
1564

Added StorageToVarsMap in D132225 but hadn't gotten round to updating this yet. Done.

1630

The DenseMap::lookup docu-comment says Return the entry for the specified key, or a default constructed value if no such entry exists so I figured it was ok.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/id.ll
4–6

Good point, done.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll
176

This test is just checking that the inlined stores are tracked correctly (e.g. the dbg.assign for f5_param that already exists in f5 prior to inlining isn't checked for here either). I think there's value in keeping this test targeted - wdyt?

jmorse accepted this revision.Nov 17 2022, 6:25 AM

LGTM with a couple of suggestions. The bigger comment is me asking "have you considered what happens when the inliner deletes random elements of assignment tracking due to pruning" -- I don't think there's anything that would be broken, just checking that we have a common understanding.

llvm/lib/Transforms/Utils/CloneFunction.cpp
719–720

What do you mean by survived the inlining?

Blocks that are determined to be unreachable during inlining are pruned away and, presumably, their debug intrinsics don't get cloned into the new function, hence it's possible for the lookup of DVI in VMap to be null. I think I wrote this comment to see if that was a code path you'd considered, in case dbg.assigns need to behave differently when things are pruned.

It's possible for blocks containing dbg.assigns to be pruned, but that should act the same as any old dead code elimination. It's probably not possible for blocks containing allocas with DIAssignIDs to be pruned but corresponding dbg.assigns not be pruned -- the dbg.assigns will be in blocks dominated by the alloca. Presumably nothing goes wrong if there are dbg.assigns with linked stores that don't get cloned, or vice versa.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1602–1604

Would a clearer test be !DAI->getDebugLoc().getInlinedAt()? (I think they're basically the same thing, doesn't really matter)

llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll
223

Check for existence of "inlinedAt" too, even if we don't really care about the node?

This revision is now accepted and ready to land.Nov 17 2022, 6:25 AM
This revision was landed with ongoing or failed builds.Nov 18 2022, 3:56 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked 4 inline comments as done.

Thanks!

llvm/lib/Transforms/Utils/CloneFunction.cpp
719–720

Yeah, that should all be okay.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1602–1604

That looks right (well, inverted) - done.

llvm/test/DebugInfo/Generic/assignment-tracking/inline/inline-stores.ll
223

We actually don't want inlinedAt here - the dbg.assign that is inserted is for the caller's variable (and the variable and dbg intrinsic scopes need to match). I've updated the CHECK line to explicitly check against seeing an inlinedAt field.