[PATCH] D56568: [AliasSetTracker] Store AliasResult and pass it on mergeSetIn.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/AliasSetTracker.cpp:60
 
-  if (Alias == SetMustAlias) {
-    // Check that these two merged sets really are must aliases.  Since both
-    // used to be must-alias sets, we can just check any pointer from each set
-    // for aliasing.
-    AliasAnalysis &AA = AST.getAliasAnalysis();
-    PointerRec *L = getSomePointer();
-    PointerRec *R = AS.getSomePointer();
-
-    // If the pointers are not a must-alias pair, this set becomes a may alias.
-    if (AA.alias(MemoryLocation(L->getValue(), L->getSize(), L->getAAInfo()),
-                 MemoryLocation(R->getValue(), R->getSize(), R->getAAInfo())) !=
-        MustAlias)
-      Alias = SetMayAlias;
-  }
+  if (Alias == SetMustAlias && AR != MustAlias)
+    Alias = SetMayAlias;
----------------
I don't believe this is correct.  The problem is that you're replacing an alias check between two sets with one between a one set and a pointer.  We know that the pointer must become a member of the set, and with your new API, we know that it's a member of that set.  However, we *don't* know it's a member of the other set.

Consider adding the pointer {p, 8} to the following sets:
{{p, 16}}, and {{p, 8}}.

Your code would conclude that the result set {{p, 16}} is MustAlias, which is incorrect.

A cleanup which would fix this would be to add the pointer *before* merging alias sets, not after.



================
Comment at: lib/Analysis/AliasSetTracker.cpp:179
 ///
-bool AliasSet::aliasesPointer(const Value *Ptr, LocationSize Size,
-                              const AAMDNodes &AAInfo,
-                              AliasAnalysis &AA) const {
+AliasResult AliasSet::aliasesPointer(const Value *Ptr, LocationSize Size,
+                                     const AAMDNodes &AAInfo,
----------------
The changes in signature to this function are obviously useful regardless of the rest of the patch.  Feel free to separate them and commit without further review.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56568