Page MenuHomePhabricator

[AliasSetTracker] Update AAInfo check.
Needs ReviewPublic

Authored by asbirlea on Sep 6 2019, 3:31 PM.

Details

Reviewers
hfinkel
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.

Diff Detail

Event Timeline

asbirlea created this revision.Sep 6 2019, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 3:31 PM
bjope added a subscriber: bjope.Sep 6 2019, 11:44 PM
hfinkel added inline comments.Sep 9 2019, 6:09 AM
include/llvm/Analysis/AliasSetTracker.h
91–92

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 marked an inline comment as done.Sep 9 2019, 7:43 AM
asbirlea added inline comments.
include/llvm/Analysis/AliasSetTracker.h
91–92

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 added inline comments.Sep 9 2019, 8:15 PM
include/llvm/Analysis/AliasSetTracker.h
91–92

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?

91–92

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

91–92

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?

asbirlea updated this revision to Diff 219596.Sep 10 2019, 1:37 PM
asbirlea marked 5 inline comments as done.

Update based on comments.

include/llvm/Analysis/AliasSetTracker.h
91–92

Would you mind elaborating?

91–92

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–92

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 added inline comments.Sep 10 2019, 4:25 PM
include/llvm/Analysis/AliasSetTracker.h
91–92

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 marked 3 inline comments as done.Sep 11 2019, 4:09 PM
asbirlea added inline comments.
include/llvm/Analysis/AliasSetTracker.h
91–92

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.

Gentle ping on this.