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

Stephen Tozer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 1 08:31:22 PDT 2022


StephenTozer added a comment.

Still reviewing, with possibly higher-level comments coming, but just putting forward some of the smaller stuff now.



================
Comment at: llvm/include/llvm/CodeGen/AssignmentTrackingAnalysis.h:25
+
+/// Datastructure describing the variable locations in a function. Used as the
+/// result of the AssignmentTrackingAnalysis pass. Essentially read-only
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:57-58
+/// Helper class to build FunctionVarLocs, since that class isn't easy to
+/// modify. TODO: There's not a great deal of value in the split, it could be
+/// worth merging the two classes.
+class FunctionVarLocsBuilder {
----------------
I think the main value is in having FunctionVarLocs be a read-only interface, which I think makes the split worthwhile.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:63
+  // Use an unordered_map so we don't invalidate iterators after
+  // insert/modificaitons.
+  std::unordered_map<const Instruction *, SmallVector<VarLocInfo>>
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:177
+    unsigned BlockEnd = VarLocRecords.size();
+    // Record t
+    if (BlockEnd - BlockStart != 0)
----------------
Could be a little more verbose.


================
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);
+}
----------------
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.


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


================
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);
+    }
----------------
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.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:311
+        : Status(Status), ID(Value), Source(Source) {
+      // If the Status is Kown then we expect there to be a Value.
+      assert(Status != Known || Value != nullptr);
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:330
+
+  static bool equ(const AssignmentMap &A, const AssignmentMap &B) {
+    if (A.size() != B.size())
----------------
Slight rename request; either this or anything along these lines.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:353
+    /// NOTE: LiveLoc is not derivable from StackHomeValue and DebugValue
+    /// values because the model is simplistic, representing PHIs and unkown
+    /// locations with the same value (UnknownOrPhi). The elements of a PHI of
----------------



================
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) {
----------------
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.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:591
+      DIExpression *Expr = DAI->getAddressExpression();
+      assert(Expr->getFragmentInfo().hasValue() == false &&
+             "fragment info should be stored in value-expression only");
----------------
Nit, but I think you can just use the boolean value of an optional for this?


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


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:692-693
+                                                      Info.SizeInBits);
+      if (!R)
+        assert(false && "unexpected createFragmentExpression failure");
+      DIE = R.getValue();
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:732
+    // that is linked to a store.
+    assert(VarsWithStackSlot->count(getAggregate(DAI)));
+
----------------
Could do with a shortened version of the comment as an assert message here.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:761
+      case LocKind::Val: {
+        // The value memory in memory has changed but we're not currently using
+        // the memory location. Do nothing.
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:974
+  // NoneOrPhi (joining two values is a Phi).
+  if (A.Status != B.Status || A.ID != B.ID)
+    return Assignment::makeNoneOrPhi();
----------------
This check looks identical to the definition of `Assignment::operator!=`, could use that instead (though I think it should also be a distinct function instead of an operator, see comment above).


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

https://reviews.llvm.org/D136320



More information about the llvm-commits mailing list