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

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

Orlando added a comment.

Many thanks for taking this review on. I've answered some of your questions in line.



================
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:
> 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.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:275-279
+    bool operator==(const Assignment &Other) const {
+      // Don't include Source in the equality check. Assignments are
+      // defined by their ID, not debug intrinsic(s).
+      return std::tie(Status, ID) == std::tie(Other.Status, Other.ID);
+    }
----------------
StephenTozer wrote:
> Does this behaviour need to be performed by `operator==`? I see it's used explicitly in `hasVarWithValue` to determine whether a valid stack home location exists for a variable, and also in `equ` to compare AssignmentMaps, but before looking for the definition for this operator I was a bit confused as to when that function would ever return true, since the `Source` field here would be different between DbgDefs and MemDefs. It's a bit of a surprise that `operator==` doesn't compare all of the fields in this class, especially since this is close to being a POD-type; if this isn't needed to make some template class functions work, could this be changed to be a distinct function `isJoinableWith` (or another name if you see fit)?
SGTM


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:309-313
+    Assignment(S Status, DIAssignID *Value, DbgAssignIntrinsic *Source)
+        : Status(Status), ID(Value), Source(Source) {
+      // If the Status is Kown then we expect there to be a Value.
+      assert(Status != Known || Value != nullptr);
+    }
----------------
StephenTozer wrote:
> Minor typo. Also, is there a reason that the argument assigned to `ID` is called `Value`? Asking more to actually understand the name assuming that there is a reason, although if there isn't any particular reason then I'd suggest changing the name.
Hmmm, no reason that I can remember. I think it's probably just a prototype-hangover - I'll change it.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:535-538
+/// Return true if Var exists in A and B and has the same value.
+static bool hasVarWithValue(VariableID Var,
+                            AssignmentTrackingLowering::Assignment AV,
+                            AssignmentTrackingLowering::AssignmentMap &M) {
----------------
StephenTozer wrote:
> Comment seems out-of-date w.r.t. the arguments; would it be more accurate as "Return true if Var has an assignment in M equal to AV." or something similar?
> Also, the name feels //slightly// misleading here, as "Value" might be reasonably interpreted as referring to a `Value`, whereas this function only cares about the Status and DIAssignID; seems similar to the use of `Value` in the `Assignment` constructor above.
Yep you are right there. Agree that the name is misleading, I'll change that.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:677-679
+    // Use an assignment ID that nothing can match against (the instruction has
+    // no DIAssignID) - this instruction is treate as both a dbg and mem def (of
+    // the same value), meaning the memory location is used.
----------------
StephenTozer wrote:
> Not sure I understand the first part of this comment, "Use an assignment ID that nothing can match against" - is this just referring to the fact that there is no DIAssignID, so `makeNoneOrPhi` must be used to create the `Assignment`? Also, minor typo.
> is this just referring to the fact that there is no DIAssignID, so makeNoneOrPhi must be used to create the Assignment

Pretty much. By "nothing can match against [it]" I meant `hasVarWithValue` will return false now for `Known` `AssignmentValue`s. I am not sure this extra commentary is necessary though - I think I'll cut it down.


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

https://reviews.llvm.org/D136320