[PATCH] D133295: [Assignment Tracking][12/*] Account for assignment tracking in mem2reg

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:133-140
+    // Insert dbg.value intrinsics for variables that are using
+    // assignment tracking but don't have a dbg.assign linked to
+    // this store.
+    for (auto *DAI : DbgAssigns) {
+      if (Ignore.contains(DebugVariable(DAI)))
+        continue;
+      ConvertDebugDeclareToDebugValue(DAI, ToDelete, DIB);
----------------
Orlando wrote:
> jmorse wrote:
> > 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.
> I think you understand this correctly and I have updated the comment to hopefully better explain. How does this look?
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133295/new/

https://reviews.llvm.org/D133295