[AliasSetTracker] Update AAInfo check.

[asbirlea]

Summary

The correction of the AAInfo check in r371139 resolved a bug but may be
too restrictive if existing MDNodes are already null.
Also, AAInfo should either be a tombstone if a mismatch exists or the
old AAInfo otherwise.

[hfinkel]

include/llvm/Analysis/AliasSetTracker.h

88

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.

[asbirlea]

include/llvm/Analysis/AliasSetTracker.h

88

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?

[hfinkel]

include/llvm/Analysis/AliasSetTracker.h

87

Given what we know about the code below, is this sound?

88

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?

91

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?

[asbirlea]

Update based on comments.

include/llvm/Analysis/AliasSetTracker.h

87

Would you mind elaborating?

88

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.

91

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.

[hfinkel]

include/llvm/Analysis/AliasSetTracker.h

87

If an AAMD field going from non-null to null requires a merge, why does going from null to non-null not require a merge?

[asbirlea]

include/llvm/Analysis/AliasSetTracker.h

87

I don't think in this case we ever go from null to non-null.
It seems right after some thinking to do merge (set to null) when the fields are different vs tombstone key, so I updated below.

[asbirlea]

Gentle ping on this.

include/llvm/Analysis/AliasSetTracker.h

bool updateSizeAndAAInfo(LocationSize NewSize, const AAMDNodes &NewAAInfo) {
       if (NewSize != Size) {
         LocationSize OldSize = Size;
         Size = isSizeSet() ? Size.unionWith(NewSize) : NewSize;
         SizeChanged = OldSize != Size;
       }
 
       if (AAInfo == DenseMapInfo<AAMDNodes>::getEmptyKey())
         // We don't have a AAInfo yet. Set it to NewAAInfo.
         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 ||
+            AAInfo.NoAlias != NewAAInfo.NoAlias) {
           AAInfo = DenseMapInfo<AAMDNodes>::getTombstoneKey();
           SizeChanged = true;
-        }
-        AAInfo = Intersection;
-      }
       return SizeChanged;
     }
 
     LocationSize getSize() const {
       assert(isSizeSet() && "Getting an unset size!");
       return Size;
     }