[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 a comment.
This revision now requires changes to proceed.
Herald added a project: LLVM.

Response to outstanding discussion point inline, but I want to suggest an alternate approach for the review so that we can make progress.

The entire concern here is because we can try to merge two alias sets which *don't* contain the exact pointerrec triggering the merging.  Let's just stop doing that.  If we simple called addPointer and addUnknown on each AliasSet we found which aliased *before* merging, all of our concerns disappear.  This is what the two callers of this function - aside from merge all - actually want anyways, so let's just do that.  It should be a simple refactor, followed by a rebase of this patch, at which point this patch becomes obviously correct.  In fact, you can the entirely remove the problematic if block from mergeSetIn, rather than try to optimize it's performance.



================
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;
----------------
asbirlea wrote:
> asbirlea wrote:
> > reames wrote:
> > > 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.
> > > 
> > On a second review, I think this is correct without the change I suggested below.
> > 
> > When deciding to merge 2 sets, we do check aliasing of the pointer, so we know it belongs to both sets. The issue that may come up, as you suggest, is that the merged set may no longer be `MustAlias`, because we don't check the first "FoundSet" at the callsite.
> > But when we do the actual adding, in addPointer, there is an alias check there against "any pointer" in that set, and a "downgrade" to `MayAlias` if that is not satisfied.
> > 
> > Now, after D56613, even that check will go away. But it's being replaced with the intersection of alias information of *all* sets that were merged, including that first "FoundSet". So, if `MustAlias` is not found for *all* merged sets, then the set is downgraded to `MayAlias`.
> > 
> > I believe that is correct. Did I miss something?
> Ping.
Reading back through the (original) code, I believe the problem was valid.  Here are two example codepaths:
addUknown->findAliasSetForUnknownInst-<mergeIn
getAliasSetFor->mergeAliasSetsForPointer->mergeIn

The core problem is that the first AS is never downgraded to MayAlias if the pointer added to it aliases, but does not may alias.  We never call mergeSetIn to that first set.

Note that in both cases, the instruction being queried for *has not* been added to any alias set before merging.  Also note that the alias result passed in is with respect to *one* of the two sets, not both.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56568