[PATCH] D67305: [AliasSetTracker] Update AAInfo check.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:87
         // We don't have a AAInfo yet. Set it to NewAAInfo.
         AAInfo = NewAAInfo;
+      else if (AAInfo.TBAA != NewAAInfo.TBAA ||
----------------
hfinkel wrote:
> Given what we know about the code below, is this sound?
Would you mind elaborating?


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:88
         AAInfo = NewAAInfo;
-      else {
-        AAMDNodes Intersection(AAInfo.intersect(NewAAInfo));
-        if (!Intersection.TBAA || !Intersection.Scope ||
-            !Intersection.NoAlias) {
-          // NewAAInfo conflicts with AAInfo.
+      else if (AAInfo.TBAA != NewAAInfo.TBAA ||
+            AAInfo.Scope != NewAAInfo.Scope ||
----------------
hfinkel wrote:
> asbirlea wrote:
> > hfinkel wrote:
> > > This seems overly restrictive. The intersection might return a valid combination of the two metadata nodes (as a conservative combination). I thought that it only returned null if there's no valid way to combine them.
> > The intersection method does this:
> > ```
> >     AAMDNodes Result;
> >     Result.TBAA = Other.TBAA == TBAA ? TBAA : nullptr;
> >     Result.Scope = Other.Scope == Scope ? Scope : nullptr;
> >     Result.NoAlias = Other.NoAlias == NoAlias ? NoAlias : nullptr;
> >     return Result;
> > ```
> > This is its only callsite.
> > I don't really know what a valid combination of two metadata nodes is TBH.
> > 
> > The "before" state was "if all fields are null, use tombstonekey, otherwise, use above intersection (fields set to null if different)". This is incomplete because any different field may need to merge sets (`return SizeChanged = true;`).
> > 
> > After I amended the above (current state), now we may return tombstonekey even when a field was nullptr to begin with in both `AAInfo` and `NewAAInfo`. Hence the change below to check for any difference.
> > 
> > Could you help me out with suggesting a less restrictive check?
> > 
> First, in general, I think that we really need better comments in this function. It does not document the meaning of its return value or arguments, and clearly this is non-trivial.
> 
> Second, SizeChanged now seems like a somewhat-misleading name for this variable (because it is not just the size). We should probably rename it.
> 
> Third, the return value of this function is not always used. In AliasSet::addPointer it is ignored (and, thus, does not affect the subsequent behavior of adding the pointer to the set). In AliasSetTracker::getAliasSetFor it causes merging when true. Maybe we want different behaviors in these different cases?
> 
> 
Updated for the first and second comments.
For the third, I think this is WAI.  The other calls are updating an existing Entry; other needed updates were already done in these cases (a potential merge, or we're in saturated mode).
I tried to better document this in the method.


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:91
+            AAInfo.NoAlias != NewAAInfo.NoAlias) {
           AAInfo = DenseMapInfo<AAMDNodes>::getTombstoneKey();
           SizeChanged = true;
----------------
hfinkel wrote:
> Why can't we keep the fields that are the same? Perhaps this was the problematic behavior, but we should at least have some comments explaining why we can't.
> 
> In the bug report (PR42969), you wrote:
> > The metadata is correct in both cases. In the vector block the load/store do not alias because of the noscope metadata added by vectorization (vectorization "guarantees" the two do not alias even though basicaaa would find they do). In the scalar block they alias due to basicaa. The metadata was simply not being checked properly. Even if we can prove no-alias between two pointers, if a metadata change occurs, then we need to check if we need to merge the sets.
> 
> And so part of the situation here may be phrased as the AST supporting context/flow-insensitive AA?
I think it's correct to keep the fields that are the same. 

> And so part of the situation here may be phrased as the AST supporting context/flow-insensitive AA?
Yes.
I'm not sure if a correct situation exists where contradictory AAInfo can break the AST...it's probably beyond the scope of this patch, but worth thinking about it.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D67305