[PATCH] D42381: [DA] Correct size parameter from dependency analysis to AA

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Analysis/DependenceAnalysis.cpp:629
   const Value *BObj = GetUnderlyingObject(B, DL);
-  return AA->alias(AObj, DL.getTypeStoreSize(AObj->getType()),
-                   BObj, DL.getTypeStoreSize(BObj->getType()));
+  return AA->alias(AObj, BObj);
 }
----------------
dmgreen wrote:
> nlopes wrote:
> > hfinkel wrote:
> > > > It's possible that the size here should be the size of the thing that AObj points to, as opposed to unknown
> > > 
> > > There are two problems here, and I think that we can fix them both. What we're really trying to do here is to identify disjoint underlying objects.
> > > 
> > >  * First, GetUnderlyingObject might not always return the underlying object (because it has a recursion-depth cutoff). Thus, we need to validate the fact that GetUnderlyingObject actually did return such a thing. So we really want to do:
> > > 
> > > 
> > >   if (!isIdentifiedObject(AObj) || !isIdentifiedObject(BObj))
> > >     return true;
> > > 
> > > 
> > >  * At that point, the sizes are essentially irrelevant. and we should pass MemoryLocation::UnknownSize as the size. However, at that point, since we have unique underlying objects, we can just compare them, so we can just do (there's no further value in calling into AA):
> > > 
> > > 
> > >   return AObj == BObj;
> > > 
> > > 
> > I agree with Hal's comment. Seems like the way to go.
> This sounds good. I like the simplification. I've updated things to something that may still be wrong.
> 
> As far as I understand, this is returning a tristate:
>  MustAlias - Do dependency analysis.
>  NoAlias - No dependence is possible.
>  MayAlias - Don't know anything - a confused dependence.
> 
> A lot of the tests use function arguments as input (as will real code). If AObj and BObj are the same argument, we are obviously mustalias, even if that isn't an identified object. So I think this is now still correct even if we hit the recursion limit, the mustalias will still be valid.
> 
> Also (and this might be something that doesn't come up very often) what if one argument has a noalias attribute, but another doesn't (or one is a alloca, the other an argument). We can still prove noalias there?
> 
> Also I was hoping to get TBAA working here. See D42382. As in - if the loads/stores of the Src and Dst are different types, we know there is no alias, so no dependency. Any ideas what the best bet there would be? Call alias on the Src and Dst? Work with the TBAA metadata somehow? (I'm not sure that's possible)
> Also (and this might be something that doesn't come up very often) what if one argument has a noalias attribute, but another doesn't (or one is a alloca, the other an argument). We can still prove noalias there?

No, although you could add that as a special case (i.e., one value is an argument (or global value) and for the other isIdentifiedFunctionLocal returns true.

> Also I was hoping to get TBAA working here.

Good point. This will require a slightly larger change, but seems worthwhile. You'll want to collect the AA metadata and then construct a memory location using that metadata but using MemoryLocation::UnknownSize, and then to an AA query using those memory locations (this will effectively check the underlying objects, but also make use of the metadata). I recommend that you change this function to accept to MemoryLocation references. Get these from the original accesses using MemoryLocation::get, and then inside this function form two new MemoryLocation objects, one from each original, MemoryLocation(LocA.Ptr, MemoryLocation::UnknownSize, LocA.AATags) (or something equivalent).


https://reviews.llvm.org/D42381