[PATCH] D119078: [LAA, LV] Add initial support for pointer-diff memory checks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:312
+  }
+  auto *Diff = SE->getMinusSCEV(SinkStartInt, SrcStartInt);
+  DiffChecks.emplace_back(Diff, AllocSize);
----------------
reames wrote:
> This bit makes me uncomfortable.  The recent direction of changes to SCEV seem to be leading us towards disallowing pointer subtractions involving distinct memory objects.  This change very much relies on the historical behavior of doing an implicit inttoptr.  See the comment of doing getMinusSCEV.
> 
> In fact, I think you are accidentally only applying this new logic in the case where we can prove the two accesses share a common base object.
> 
> If you're okay documenting that restriction, we can punt the pointer_subtract semantics question one step further down the road.  
> 
> 
> This bit makes me uncomfortable. The recent direction of changes to SCEV seem to be leading us towards disallowing pointer subtractions involving distinct memory objects. This change very much relies on the historical behavior of doing an implicit inttoptr. See the comment of doing getMinusSCEV.

Thanks for the heads up, I missed that! The latest version of the patch has the pointer-difference part stripped, but I'll keep that in mind for the follow-up. The only reason to use SCEV here was to take advantage of SCEVExpander for convenience, but the code can also be generated without SCEV.

> In fact, I think you are accidentally only applying this new logic in the case where we can prove the two accesses share a common base object.

We should only compute the difference if the bases are may-alias (or the step is not constant). I *think* `needsChecking` should skip any pointer-group pairs with a common base.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1634
+                             ConstantInt::get(Ty, IC * C.second));
+    auto *Diff = Expander.expandCodeFor(C.first, Ty, Loc);
+    Value *IsConflict =
----------------
reames wrote:
> Somewhat an aside, but this is one more place where having a general SCEV note for a predicate would seem useful.  In this case, we'd cache the overlap predicate directly, and the emission code wouldn't need to be so closely coupled.  
The notion of a predicate in SCEV would indeed be convenient here (and at the other places where we generate runtime checks). It would/should also allow for more convenient checking if the check can be simplified by SCEV.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119078