[PATCH] D133294: [Assignment Tracking][11/*] Update RemoveRedundantDbgInstrs

[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 some cosmetic nits.



================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:436
+      auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI);
+      bool IsDbgValueKind = (!DAI || at::getAssignmentInsts(DAI).empty());
+
----------------
Probably wants a comment to summarise the purpose, i.e., "dbg.assigns with no linked instructions can be...". I know there's a similar comment below, but IMO it makes more sense to put that comment up here.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:498
+  for (auto &I : *BB) {
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
+      auto *DAI = dyn_cast<DbgAssignIntrinsic>(DVI);
----------------
Early-continue better than indentation IMO.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:507
+        } else if (DAI) {
+          ToBeRemoved.push_back(DVI);
+        }
----------------
Best to be 100% explicit -- push DAI, and change the type of the container to be DbgAssignInst?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:513
+
+  for (auto &Instr : ToBeRemoved)
+    Instr->eraseFromParent();
----------------
Nit, `auto *` rather than a reference to the pointer? I vaguely recall some past compilers issuing diagnostics about this.


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

https://reviews.llvm.org/D133294