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

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

Orlando marked 4 inline comments as done.
Orlando added a comment.

In D136325#3944096 <https://reviews.llvm.org/D136325#3944096>, @jmorse wrote:

> 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?



> it only selects the bits that do overlap

`meetFragments` is merging the `LiveOut` results of preds. If all preds agree the memory location for some bits is valid (intersect of variable fragments in memory) then that information is propagated. Bits not in that intersect are not treated as located in memory. During `process` (processing a block) the opposite occurs; the interval map is built up by taking the union of "bits in memory" according to the map and the encountered debug intrinsic. Where there is an intersection in that union we need to emit debug intrinsics to explicitly re-instate the memory location for the bits outside the intersect (union minus intersect). In other worse, the bits in memory that are "not shadowed" by a subsequent overlapping location def need a new location def.

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

I don't have any to hand but I can try and dig some out.

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

I did briefly investigate this during development and chose to use IntervalMap for the reasons you've just mentioned.



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


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:309-310
+  UniqueVector<DebugAggregate> Aggregates;
+  DenseMap<const BasicBlock *, VarFragMap> LiveIn;
+  DenseMap<const BasicBlock *, VarFragMap> LiveOut;
+
----------------
jmorse wrote:
> 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?
Same as for the other patches, `LiveIn` and `LiveOut` are initialised to have a number of elements equal to the number of basic blocks, so in terms of reallocations we only pay for those done by the inner map, VarFragMap (which is a map of intervalmaps though - perhaps this is what you were talking about?).


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:585-588
+    const unsigned Base =
+        DerefOffsetInBytes && *DerefOffsetInBytes * 8 == StartBit
+            ? Bases.insert(VarLoc.V)
+            : 0;
----------------
jmorse wrote:
> 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.
`MemLocFragFill` (this pass) only works with memory locations.

a) `AssignmentTrackingLowering` (the previous pass) rewrites memory locations in terms of the base alloca
b) Variadic address component of a dbg.assign is not supported (and there are no plans for that)


So, here `VarLoc.V` is a) the base pointer for the whole variable and b) will never be variadic (even if/when the value component is updated to support variadic expressions).



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:604
+        LLVM_DEBUG(dbgs() << "- No overlaps\n");
+        FragMap.insert(StartBit, EndBit, Base);
+      } else {
----------------
jmorse wrote:
> Early return, avoid indentation?
Done, and have done the same for the outer `else` block (pulled it to the top just above here).


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

https://reviews.llvm.org/D136325