[PATCH] [LoopAccesses 1/3] Expose MemoryDepChecker to LAA users

[Adam Nemet]

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:94
@@ +93,3 @@
+/// This class works under the assumption that we already checked that memory
+/// locations with different underlying pointers are "must-not alias".
+/// We use the ScalarEvolution framework to symbolically evalutate access
----------------
hfinkel wrote:
> anemet wrote:
> > hfinkel wrote:
> > > Also, IIRC, this assumption about underlying pointers not aliasing is not quite true (especially after I introduced the alias-set-tracker-based access partitioning), but we should return an 'Unknown' dependence given pointers with different (potentially aliasing) underlying pointers. We should probably remove this statement (unless you feel it is still true).
> > Could you please elaborate why you think it's not true?  I thought it was true assuming we passed the run-time checks.
> > 
> > My understanding is that we partition the accesses first along the different alias sets and then along the different underlying pointers used.  Then we perform dependence checking within the partitions (each sharing the same alias set and underlying pointer).  I am not sure how we could see accesses with different underlying pointers in isDependent.
> > 
> > For accesses in the same alias set but using a different underlying pointer we emit a run-time check (same AliasSetId, different DependencySetId).
> > 
> > Am I missing something?
> > My understanding is that we partition the accesses first along the different alias sets and then along the different underlying pointers used.
> 
> We do, but my point is that I don't see what in this class assumes, or depends on the fact, that we've done that. If we ask for the dependence of two pointers with different underlying values, then we should hit this code:
> 
>   const SCEVConstant *C = dyn_cast<SCEVConstant>(Dist);
>   if (!C) {
>     DEBUG(dbgs() << "LAA: Dependence because of non-constant distance\n");
>     ShouldRetryWithRuntimeCheck = true;
>     return true;
>   }
> 
> in isDependent. If this is not a pre-condition to using the class, then we should remove or rephrase this comment (because it sounds like a precondition).
> 
OK, I certainly agree that it does sound like a precondition and that is a bit strong.

On the other hand, if we were to rely on isDepedent to answer these queries we would trigger the whole ShouldRetryWithRuntimeCheck path which completely disregards DependencySets.  So we could end up with more memchecks in order the vectorize the loop.

So it's probably better to think of it as a precondition not for correctness but for the proper division of work between MemoryDepChecker and AccessAnalysis/RuntimePointerCheck.

This is probably a bit too subtle to capture in the class comment though...  Perhaps something like:

"(We handle may-aliasing accessed using different underlying pointers in AccessAnalysis/RuntimePointerCheck by emitting memchecks.)"

http://reviews.llvm.org/D8113

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/