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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Comments aside, LGTM.



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:35
+/// Option for debugging the pass, determines if the memory location fragment
+/// fillling happens after generating the variable locations.
+static cl::opt<bool> EnableMemLocFragFill("mem-loc-frag-fill", cl::init(true),
----------------



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:248
+  // Check this ends in deref or deref then fragment.
+  if (Elements[NextElement] == dwarf::DW_OP_deref) {
+    if (NumElements == NextElement + 1)
----------------
This could be inverted and made into an early return.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:300
+  FragsInMemMap::Allocator IntervalMapAlloc;
+  using Map = DenseMap<unsigned, FragsInMemMap>;
+
----------------
A more descriptive name might be useful, painful as it is for a map of maps; even just `VarFragMap` or `AggFragMap` would be reasonable I think.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:319
+
+  static bool equ(const FragsInMemMap &A, const FragsInMemMap &B) {
+    auto AIt = A.begin(), AEnd = A.end();
----------------
`equ` functions may be worth renaming.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:452
+
+  void meetVars(Map &A, Map &B) {
+    // Meet A and B.
----------------
Might be worth putting in a function comment that `A` is modified to contain the result of the meeting, while `B` is not modified; if you can think of good variable names to represent that as well (I can't) then that would be a good idea as well.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:531
+                    DebugLoc DL) {
+    if (!Base)
+      return;
----------------
Maybe worth adding an `assert(StartBit < EndBit && "Cannot create fragment of size <= 0")` here as a guard against any future modifications to this pass (it seems like a possible result of some incorrect handling of intervals; there were a few cases in this patch I //thought// this error might appear until I investigated further).


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:545
+
+  void addDef(VarLocInfo VarLoc, Instruction &After, BasicBlock &BB,
+              Map &LiveSet) {
----------------
Is the `After` parameter misnamed? As far as I can tell the intent is that this Def becomes live before `After`, which technically makes sense (the instruction comes "after" the def) but uses the opposite naming convention to elsewhere, where the paramater is named `Before` as-in "The instruction we're inserting before".


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:576-577
+    // written in terms of the base pointer.
+    // TODO: Remove this condition since the fragment offset doesn't always
+    // equal the offset from base pointer (e.g. for a SROA-split variable).
+    const auto DerefOffsetInBytes = getDerefOffsetInBytes(DIExpr);
----------------
It looks like the condition has indeed been removed, unless the actual conditional check appears elsewhere in this pass?


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:609
+        auto LastOverlap = FragMap.find(EndBit);
+        bool IntersectEnd = LastOverlap.valid() && LastOverlap.start() < EndBit;
+
----------------
Should this be `LastOverlap.stop() > EndBit`? I could have some misunderstandings about how this works, but to my understanding since this is a half-open interval map, if `LastOverlap.start() >= EndBit` then the interval map wouldn't consider it an overlap at all (e.g. `[0, 16)` does not overlap with `[16, 32)`).


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:615-618
+          // [ f ]
+          // [ - i - ]
+          // +
+          // [ f ][ i ]
----------------
I believe this example is actually incorrect - `IntersectStart` and `IntersectStop` are only true if the existing fragment extends beyond the start //and// end of this fragment, i.e.
```
     [ f ]
[  -   i   -  ]
+
[ i ][ f ][ i ]
```
In the example given in this comment, `IntersectStart` would be false because this fragment and the existing fragment have the same start bit, and so we would (correctly) fall through to the else block below.


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

https://reviews.llvm.org/D136325