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

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

Orlando added a comment.

Thanks @StephenTozer



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:531
+                    DebugLoc DL) {
+    if (!Base)
+      return;
----------------
StephenTozer wrote:
> 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).
SGTM!


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:545
+
+  void addDef(VarLocInfo VarLoc, Instruction &After, BasicBlock &BB,
+              Map &LiveSet) {
----------------
StephenTozer wrote:
> 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".
Yeah, sorry about that. This changed a few times during development.


================
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);
----------------
StephenTozer wrote:
> It looks like the condition has indeed been removed, unless the actual conditional check appears elsewhere in this pass?
That's the ternary operator condition below.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:615-618
+          // [ f ]
+          // [ - i - ]
+          // +
+          // [ f ][ i ]
----------------
StephenTozer wrote:
> 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.
Yeah looks like you're right. Nice catch - I appreciate the keen-eyed review!


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

https://reviews.llvm.org/D136325