[PATCH] D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA

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

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:201
+    // If we haven't created a DIAssignID ID do that now and attach it to Inst.
+    if (!NewID) {
+      NewID = DIAssignID::getDistinct(Ctx);
----------------
jmorse wrote:
> I believe this is always true, probably because of some refactors.
It's initialized to `nullptr` outside of this loop.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3051
+        if (isa<Constant>(II.getLength())) {
+          assert((NewBeginOffset - NewAllocaBeginOffset == 0 ||
+                  at::getAssignmentMarkers(&II).empty()) &&
----------------
jmorse wrote:
> First option simplifiable to `NewBeginOffset == NewAllocaBeginOffset`? It's not immediately obvious what this is checking, and I feel it might be easier to read if we name the other proposition "NoAssignmentMarkers" or something. That might also allow you to condense or invert the control flow. The for loop is dependent on !NoAssignmentMarkers, for example.
I think this blob represents me not being sure what to do here at the time. I don't think the for loop below is correct - based on other similar changes in the patch stack that probably needs to be a replaceUsesOfWith style update. It turns out I didn't have test coverage for this. sitrep: I've now got a test that stimulates this codepath (not uploaded) - but not yet managed to get one that let's us inspect the state at this point (it goes through another round of SROA-ing).


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

https://reviews.llvm.org/D133296