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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looks good, although I think some of the logic at one of the inline comments can be simplified.

Once again I haven't looked in depth at the tests -- clearly they're there to establish good coverage, but each is pretty complicated and it's hard to judge whether all the paths in SROA are covered. I think that's a necessary evil -- this is a new thing, after all, and the tests are necessarily complicated.



================
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);
----------------
I believe this is always true, probably because of some refactors.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3051
+        if (isa<Constant>(II.getLength())) {
+          assert((NewBeginOffset - NewAllocaBeginOffset == 0 ||
+                  at::getAssignmentMarkers(&II).empty()) &&
----------------
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.


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

https://reviews.llvm.org/D133296