[PATCH] D145558: [Assignment Tracking][NFC] Use BitVectors as masks for SmallVectors rather than using DenseMaps

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

It might be worth including the rationale in the description/commit message, like you've done in other similar changes. I assume it is a performance improvement?



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:998-1005
+    int VarID = Mask.find_first();
+    while (VarID != -1) {
       // Check that the assignment value is the same.
-      if (!AV.isSameSourceAssignment(R->second))
+      if (!A[VarID].isSameSourceAssignment(B[VarID]))
         return false;
+      VarID = Mask.find_next(VarID);
     }
----------------
It's a matter of taste, but I find the `algorithm`s approach easier to read. Alternatively, I think this loop can at least use the `set_bits()` range rather than `find_first`+`find_next`


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1014-1018
+    /// Dominating assignment to memory for each variable, indexed by
+    /// VariableID.
     AssignmentMap StackHomeValue;
-    /// Dominating assignemnt to each variable.
+    /// Dominating assignemnt to each variable, indexed by VariableID.
     AssignmentMap DebugValue;
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1035-1038
+    const Assignment &getDebugValue(VariableID Var) const {
+      assert(isVariableTracked(Var) && "Var not tracked in block");
+      return DebugValue[static_cast<unsigned>(Var)];
+    }
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1145
   /// Return true if \p Var has an assignment in \p M matching \p AV.
-  bool hasVarWithAssignment(VariableID Var, const Assignment &AV,
-                            const AssignmentMap &M);
+  bool hasVarWithAssignment(VariableID Var, const BitVector &Mask,
+                            const Assignment &AV, const AssignmentMap &M);
----------------
I think this starts to get a little confusing, as there isn't a direct connection between the two arguments (`Mask` and `M`). Maybe they should be adjacent in the parameter list?

The only other suggestion I have is to hide all the data members in `BlockInfo`. There is already `isVariableTracked` and `getDebugValue`, I would just extend this to cover ever operation you need. All of the mask handling will then be adjacent and abstracted away, so the rest of the code reads easier and there is no danger of missing a mask check or update.

I'm leaving a bunch of edit suggestions, but feel free to ignore them! I was just going through the exercise of writing one possible implementation to prove to myself it is an actual improvement, YMMV.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1165-1167
+    LiveSet->VariableIDsInBlock.set(Idx);
     VarsTouchedThisFrame.insert(Var);
+    LiveSet->LiveLoc[Idx] = K;
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1178
 AssignmentTrackingLowering::getLocKind(BlockInfo *LiveSet, VariableID Var) {
-  auto Pair = LiveSet->LiveLoc.find(Var);
-  assert(Pair != LiveSet->LiveLoc.end());
-  return Pair->second;
+  return LiveSet->LiveLoc[static_cast<unsigned>(Var)];
 }
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1184-1186
+    unsigned Idx = static_cast<unsigned>(Var);
+    LiveSet->VariableIDsInBlock.set(Idx);
+    LiveSet->StackHomeValue[Idx] = AV;
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1202-1204
+    unsigned Idx = static_cast<unsigned>(Var);
+    LiveSet->VariableIDsInBlock.set(Idx);
+    LiveSet->DebugValue[Idx] = AV;
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1226-1247
 bool AssignmentTrackingLowering::hasVarWithAssignment(VariableID Var,
+                                                      const BitVector &Mask,
                                                       const Assignment &AV,
                                                       const AssignmentMap &M) {
-  auto AssignmentIsMapped = [](VariableID Var, const Assignment &AV,
-                               const AssignmentMap &M) {
-    auto R = M.find(Var);
-    if (R == M.end())
+  auto AssignmentIsMapped = [&](VariableID Var, const Assignment &AV,
+                                const AssignmentMap &M) {
+    unsigned Idx = static_cast<unsigned>(Var);
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:1432-1433
     // assignment has a matching Assignment.
-    if (hasVarWithAssignment(Var, AV, LiveSet->DebugValue)) {
+    if (hasVarWithAssignment(Var, LiveSet->VariableIDsInBlock, AV,
+                             LiveSet->DebugValue)) {
       // The StackHomeValue and DebugValue for this variable match so we can
----------------



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

https://reviews.llvm.org/D145558