[PATCH] D87778: [MemorySSA] Be more conservative when traversing MemoryPhis.

Sun Nov 15 11:48:23 PST 2020

nikic added inline comments.

================
Comment at: llvm/include/llvm/Analysis/MemorySSA.h:1251
+      // after the pointer are considered as clobbers, which is important to
+      // catch loop carried dependences.
+      if (Location.Ptr &&
----------------
fhahn wrote:
> nikic wrote:
> > As the comment correctly states, an unknown size guarantees that locations **after** the pointer are considered as clobbers. However, locations **before** it are not. Could there still be an issue for decrementing pointer loops here?
> It appears that currently things work out fine with decrementing pointers because of how BasicAA deals with negative offsets. And there are also a few checks that bail out if either size is unknown. I am not familiar enough with the walking code/the basicAA logic for those cases to judge if we just get lucky. In any case, I put up 7fa8b629208c which adds the decrement version of the test added here. Without the patch, it has the same issue as the increment version.
Thanks for adding the test case! I believe that we are indeed getting lucky right now, in that BasicAA is currently slightly over-conservative when it comes to negative offsets, and it is thus hard to reproduce any issues. When I apply D91482 over the new test, it produces an incorrect liveOnEntry use. Assuming my understanding of LocationSize::unknown() is correct, that's an issue with this MemorySSA code rather than the patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87778