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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


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


================
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();
+  }
----------------
jmorse wrote:
> Need to use `at::getAssignmentMarkers(AI)` again because `DbgAssigns` filters some of the dbg.assigns out?
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.


================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:692
       AllocaDbgUsers[AllocaNum] = Info.DbgUsers;
+    AllocaATInfo[AllocaNum] = Info.AssignmentTracking;
 
----------------
jmorse wrote:
> 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.
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.


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

https://reviews.llvm.org/D133295