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

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

Orlando added a comment.

In D136320#3870269 <https://reviews.llvm.org/D136320#3870269>, @nlopes wrote:

> Please avoid using UndefValue::get as much as possible as we are trying to get rid of undef. Please use PoisonValue whenever possible.
> Thank you!

Hi @nlopes. This patch is part of the series in which we agreed this could be addressed after landing the stack: https://reviews.llvm.org/D133293#inline-1286892. That is the next item on my TODO list and I plan to make the change for all debug info modes (rather than just this new one). Is that still okay with you?

---

Thanks @StephenTozer, I've made those changes.



================
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);
+}
----------------
StephenTozer wrote:
> 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!
Aha, gotcha, thanks. Changed to use `append`.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:663
+  // Find markers linked to this alloca.
+  for (DbgAssignIntrinsic *DAI : Linked) {
+    if (DAI->getExpression()->getNumElements() != 0)
----------------
StephenTozer wrote:
> 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.
Yeah it will. I think that having multiple identical dbg.assigns linked to an alloca is unlikely though - IMO it's not worth filtering them out.


================
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
----------------
StephenTozer wrote:
> 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?
True, nice catch.


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

Not for the outer loop as `first` isn't used & causes an unused variable warning. Will do for the second (this code was written before the move to C++17!).


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1319
+        assert(isa<AllocaInst>(VarLoc.V));
+        assert(Simple);
+        VarLoc.Expr = DIExpression::get(Fn.getContext(), None);
----------------
StephenTozer wrote:
> 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; }`
I've added a comment - if you still think it should go then I'll remove it.


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

https://reviews.llvm.org/D136320