[PATCH] D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

General implementation of the patch LGTM!



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:188-189
+  Variables.push_back(DebugVariable(nullptr, None, nullptr));
+  for (auto VarLoc : Builder->Variables)
+    Variables.emplace_back(VarLoc);
+}
----------------
Orlando wrote:
> StephenTozer wrote:
> > Is there a reason this couldn't be inserted with `.insert()`? The for-loop with `emplace_back` would only be necessary if `Variables` and `Builder->Variables` were different types I think, but I might be missing something.
> They are indeed different types - `Variables` is a SmallVector and `Builder->Variables` is a `UniqueVector` (I figured there was no need in paying the additional memory overhead cost of a UniqueVector since we're done inserting elements at this point.
Sorry, I meant the element types of the vectors - even for different vector types, I believe `Variables.insert(Variables.begin(), Builder->Variables.begin(), Builder->Variables.end());` should work - if not, then please ignore!


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:663
+  // Find markers linked to this alloca.
+  for (DbgAssignIntrinsic *DAI : Linked) {
+    if (DAI->getExpression()->getNumElements() != 0)
----------------
Just confirming, will this loop body be executed multiple times for the same variable if the base alloca is linked to multiple DbgAssignIntrinsics? I don't think it would result in any incorrectness if that is the case, since this loop body is idempotent, but it might slow things down a little and possibly spam `dbgs()` a bit.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1235
+  SmallPtrSet<BasicBlock *, 16> Visited;
+  while (!Worklist.empty() || !Pending.empty()) {
+    // We track what is on the pending worklist to avoid inserting the same
----------------
Could remove the `!Pending.empty()` condition, since Pending is empty when we reach this loop and is asserted to be empty at the end of each iteration?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1289-1291
+  for (auto Pair : InsertBeforeMap) {
+    auto &MapVec = Pair.second;
+    for (auto Pair : MapVec) {
----------------
Nit, shadowed variables here and in the "Insert the other DEFs" loop below (could use structured bindings?)


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1319
+        assert(isa<AllocaInst>(VarLoc.V));
+        assert(Simple);
+        VarLoc.Expr = DIExpression::get(Fn.getContext(), None);
----------------
Nit: Normally not too bothered about having many asserts even when they seem obvious, but `assert(Simple)` probably isn't needed just a few statements down from `if (!Simple) { ...; continue; }`


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1396
+      // DIAssignID might get dropped from an alloca but not stores. In that
+      // case, we need to consider the variable intersting for NFC behaviour
+      // with this change. TODO: Consider only looking at allocas.
----------------



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

https://reviews.llvm.org/D136320