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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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)



================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:106
+class AssignmentTrackingInfo {
+  /// DbgAssignIntrinsics linked to the alloca.
+  SmallVector<DbgVariableIntrinsic *> DbgAssigns;
----------------
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?


================
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);
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:158-159
+    // don't provide any useful information once it is deleted.
+    for (auto *DAI : DbgAssigns)
+      DAI->eraseFromParent();
+  }
----------------
Need to use `at::getAssignmentMarkers(AI)` again because `DbgAssigns` filters some of the dbg.assigns out?


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

https://reviews.llvm.org/D133295