This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][12/*] Account for assignment tracking in mem2reg
ClosedPublic

Authored by Orlando on Sep 5 2022, 5:15 AM.

Details

Summary

The changes for assignment tracking don't require much of a deviation from existing behaviour. dbg.assign intrinsics linked to an alloca are treated much in the same way as dbg.declare users of an alloca, except that we don't insert dbg.value intrinsics to describe assignments when there is already a dbg.assign intrinsic present, e.g. one linked to a store that is going to be removed.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:15 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Sep 5 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:15 AM
jmorse added a subscriber: jmorse.Sep 7 2022, 7:34 AM

Possibly I've missed something -- there's nothing here that's converting dbg.declares to dbg.assigns, so it's up to the frontend to produce appropriate dbg.assigns initially, yes?

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
105

This will want a top-level docu-comment if there are members that are docucommented.

324

Docu-comment etc

692

The original authors saw fit to guard the copying of Info.DbgUsers by empty(), best to continue the pattern by guarding the copying of Info.AssignmentTracking.

Possibly I've missed something -- there's nothing here that's converting dbg.declares to dbg.assigns, so it's up to the frontend to produce appropriate dbg.assigns initially, yes?

Yep, in D132226 Clang injects the pass AssignmentTrackingPass, which added in D132225, into the optimization pipeline. It runs before any optimizations.

N.B. (possibly going off on a tangent here): originally, in the prototype, the initial dbg.assign-inserting was done from the front end, but the implementation basically had the same behaviour as that pass (convert dbg.declares to dbg.assigns). Using a pass has advantages in that it can be tested, and it's less invasive in Clang, and provides a cheap/easy way for front ends to try out assignment tracking. In the long run we should probably aim to deprecate the pass (and dbg.declare) and get front ends to produce the intrinsics themselves, but I think this is a good staging step.

jmorse added a comment.Sep 8 2022, 4:14 AM

A small risk here is that there are promotion-parts of mem2reg that aren't covered; I think if there are, they'll show up when tests are converted to use dbg.assigns rather than dbg.value.

In the tests where allocas are deleted, should we not be testing that the alloca reference in the dbg.assigns are replaced with undef?

Probably beyond the call of duty, but there's no need for the tbaa metadata to be part of these tests, and it'll reduce unrelated maintenance if we can drop it.

(Otherwise looks good)

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
106

It isn't a comprehensive set though, right? From the constructor, it's a sample of one dbg.assign per DebugVariable. The purpose being that they act like a dbg.declare, the reference for creating dbg.values?

133–140

Am I right in thinking that this is where un-linked stores have an effect on variables, i.e., if we promote something off the stack and there are some unlinked stores, then they get dbg.values created for them here?

If so, IMO better to word the comment from that perspective. I think it's a style thing, but I'd flip the focus of the sentence to the open with "Stores that are not linked to dbg.assigns...", that gives the reader more context about what the purpose of this code is.

158–159

Need to use at::getAssignmentMarkers(AI) again because DbgAssigns filters some of the dbg.assigns out?

Orlando updated this revision to Diff 459055.Sep 9 2022, 7:15 AM
Orlando marked 6 inline comments as done.

+ Addressed review comments.

A small risk here is that there are promotion-parts of mem2reg that aren't covered; I think if there are, they'll show up when tests are converted to use dbg.assigns rather than dbg.value.

To the best of my knowledge I got them all, but it's definitely a risk.

In the tests where allocas are deleted, should we not be testing that the alloca reference in the dbg.assigns are replaced with undef?

Done.

Probably beyond the call of duty, but there's no need for the tbaa metadata to be part of these tests, and it'll reduce unrelated maintenance if we can drop it.

Done.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
133–140

I think you understand this correctly and I have updated the comment to hopefully better explain. How does this look?

158–159

That is true, nice catch. I've added a new function deleteAssignmentMarkers(const Instruction *) in a new patch D133576, which is now called directly in place of calls to updateForDeletedAlloca, which I've now removed.

I've modified the test single-store-alloca.ll to check this works as expected.

692

Done. I think this should be okay - if the resize performed on AllocaATInfo before this loop behaves like a std::vector::resize IIUC it should result in the AssignmentTrackingInfo element member DbgAssigns being default-initialized. So everything should be okay if the assignment is skipped.

jmorse accepted this revision.Sep 14 2022, 7:44 AM

LGTM with a microscopic wording nit that you don't need to worry abuot.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
133–140

Sounds good; you might want to shoe-horn in the fact that untracked stores should be rare, and are typically undesirable, it should set the right tone in the mind of the reader.

(A lot of these comments are about making the future reader think the right thoughts, for when someone comes and redesigns this in n years time, in which case they should ask themselves "why were untracked stores considered undesirable", and come back to the what-is-the-source-of-truth problems that have been discussed).

This revision is now accepted and ready to land.Sep 14 2022, 7:44 AM
This revision was landed with ongoing or failed builds.Nov 15 2022, 3:12 AM
This revision was automatically updated to reflect the committed changes.