[PATCH] D61890: [LiveDebugValues] End variable's range with multiple locations at block entry

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:767
+  // Variables that have multiple locations in current BB.
+  SmallPtrSet<const DIVariable *, 4> ConflictVars;
+  DenseMap<const DIVariable *, const DebugLoc *> VariablesDL;
----------------
Out of curiosity, did you empirically arrive at this number? I.e., is it above the median size of the set when compiling clang? I'm asking because LiveDebugValues is notoriously performance-sensitive.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:768
+  SmallPtrSet<const DIVariable *, 4> ConflictVars;
+  DenseMap<const DIVariable *, const DebugLoc *> VariablesDL;
 
----------------
Are you sure that using DIVariable alone as the index is the right thing to do here? Does this handle inlined DBG_VALUEs the way it's supposed to? I'm thinking about a function that is inlined twice in a row into the same basic block, for example.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:789
     // predecessor, and for all other predecessors join the Out locs.
-    if (!NumVisited)
+    // Also keep track about variables that have multiple locations
+    // so that we can close theirs range since we can't decide which location
----------------
multiple locations .. inside this BB?


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:795
+      for (auto ID : InLocsT)
+        ConflictVars.insert(VarLocIDs[ID].Var.getVar());
+    } else {
----------------
It reads strange to unconditionally insert all incoming DBG_VALUES into a set called ConflictVars.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:799
+      VarLocSet DanglingLocs = OL->second;
+      DanglingLocs.intersectWithComplement(InLocsT);
+      SmallPtrSet<const DIVariable *, 4> CurrentConf;
----------------
This looks like it could be really slow. Have you measured the performance impact? Would it be faster to have a map from Variable->Index and use a BitVector instead of a SmallPtrSet?


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

https://reviews.llvm.org/D61890