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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
103 | This will want a top-level docu-comment if there are members that are docucommented. | |
323 | Docu-comment etc | |
696 | 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. |
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.
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 | ||
---|---|---|
104 | 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? | |
131–138 | 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. | |
156–157 | Need to use at::getAssignmentMarkers(AI) again because DbgAssigns filters some of the dbg.assigns out? |
+ 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 | ||
---|---|---|
131–138 | I think you understand this correctly and I have updated the comment to hopefully better explain. How does this look? | |
156–157 | 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. | |
696 | 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. |
LGTM with a microscopic wording nit that you don't need to worry abuot.
llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp | ||
---|---|---|
131–138 | 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 will want a top-level docu-comment if there are members that are docucommented.