[PATCH] D136325: [Assignment Tracking Analysis][3/*] Memory location fragment filling

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added subscribers: CarlosAlbertoEnciso, jmorse.
jmorse requested changes to this revision.
jmorse added a comment.
This revision now requires changes to proceed.

This is well motivated and useful -- I'm having some trouble building a mental model of what the lattice looks like though. IIUC we're seeking to identify the ranges of variable location that are well defined by the fragment ranges, but, because we later drop overlapped ranges at a later date, we want to explicitly state the not-overlapped memory location so that it isn't dropped. I don't quite see how `meetFragments` achieves that, as it only selects the bits that do overlap. Is the lattice collecting all the ranges where there's genuine overlap, so that later dbg.values split their ranges across the intersection? And if so, why does addDef delete "fully contained" overlaps, shouldn't it break all definitions up?

Another concern is the high cost of copying a DenseMap of IntervalMap's each join/meet -- this is probably fine if there are only a few leaked + partially promoted variables in a function, which is typically the case, do you have any statistics on how frequently that happens in a clang build for example? This might be a scenario where we need to land+evaluate on wider code bases before refining. It also might be acceptable to have a cut-off point where there are too many leaked variable locations to track, to avoid excessive work.

One also wonders whether @CarlosAlbertoEnciso 's IntervalTree would be useful to collect and identify overlapping areas. That would probably want a "global view" of the function where all fragments are split up from the beginning, wheras I suppose taking the dataflow approach limits it to only the regions where there's some overlap.

(Hitting request changes to take it out of the review queue).



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:221-223
+/// Extract the offset used in \p DIExpr. Returns None if the expression
+/// doesn't explicitly describe a memory location with DW_OP_deref or if the
+/// expression is too complex to interpret.
----------------
Sounds good, potentially this is useful as a method of DIExpression, but let's think about that later.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:259-260
+
+  // Don't bother trying to interpret anything more complex.
+  return None;
+}
----------------
Just to check my understanding: so anything that ends with DW_OP_stack_value is going to get filtered out by this check, yes?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:281
+/// and describing a location (memory) rather than a value means we don't need
+/// to worry about splitting any values.
+/// This class performs a(nother) dataflow analysis over the function, adding
----------------
".. and so we try to recover the rest of the fragment location here."


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:309-310
+  UniqueVector<DebugAggregate> Aggregates;
+  DenseMap<const BasicBlock *, VarFragMap> LiveIn;
+  DenseMap<const BasicBlock *, VarFragMap> LiveOut;
+
----------------
Mmmmm, map of map of interval map. Reallocations of this are going to be expensive. Do you have any statistics wrt. how frequently these scenarios present themseves?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:551
+
+  void addDef(VarLocInfo VarLoc, Instruction &Before, BasicBlock &BB,
+              VarFragMap &LiveSet) {
----------------
`const VarLocInfo &`


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:585-588
+    const unsigned Base =
+        DerefOffsetInBytes && *DerefOffsetInBytes * 8 == StartBit
+            ? Bases.insert(VarLoc.V)
+            : 0;
----------------
Something unclear to me is what `VarLoc.V` is in this scenario -- it's being treated as if it's an alloca base, but the comment in patch 1 of "Needs to be value_s_ for variadic expressions." suggests that `V` is the actual value of an assignment. Commentry on the `Bases` container of what types it contains would be good.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:604
+        LLVM_DEBUG(dbgs() << "- No overlaps\n");
+        FragMap.insert(StartBit, EndBit, Base);
+      } else {
----------------
Early return, avoid indentation?


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

https://reviews.llvm.org/D136325