[polly] r219275 - [Fix] Ignore forwarding alias sets in the alias set tracker.

[Tobias Grosser]

On 08/10/2014 19:23, Johannes Doerfert wrote:
> On 10/08, Tobias Grosser wrote:
>> On 08/10/2014 04:23, Johannes Doerfert wrote:
>>> Author: jdoerfert
>>> Date: Tue Oct  7 21:23:48 2014
>>> New Revision: 219275
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=219275&view=rev
>>> Log:
>>> [Fix] Ignore forwarding alias sets in the alias set tracker.
>>>
>>> Modified:
>>>      polly/trunk/lib/Analysis/ScopInfo.cpp
>>>
>>> Modified: polly/trunk/lib/Analysis/ScopInfo.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopInfo.cpp?rev=219275&r1=219274&r2=219275&view=diff
>>> ==============================================================================
>>> --- polly/trunk/lib/Analysis/ScopInfo.cpp (original)
>>> +++ polly/trunk/lib/Analysis/ScopInfo.cpp Tue Oct  7 21:23:48 2014
>>> @@ -1294,7 +1294,7 @@ bool Scop::buildAliasGroups(AliasAnalysi
>>>
>>>     SmallVector<AliasGroupTy, 4> AliasGroups;
>>>     for (AliasSet &AS : AST) {
>>> -    if (AS.isMustAlias())
>>> +    if (AS.isMustAlias() || AS.isForwardingAliasSet())
>>>         continue;
>>>       AliasGroupTy AG;
>>>       for (auto PR : AS)
>>
>> This change unbroke the asserts on our build bots. This is nice!
>>
>> Could you explain me what forwarding alias sets are and why they are safe to
>> ignore? Specifically, why do we know that they will not contain
>> any pointers (except the forward reference).
> See the link.
>
>> I was quickly browsing the alias set tracker code, but I did not find the
>> corresponding documentation.
> http://llvm.org/doxygen/classllvm_1_1AliasSet.html#a059eb44c592d8d82ea2a59069bc09ee3

Thanks.

"isForwardingAliasSet - Return true if this alias set should be ignored 
as part of the AliasSetTracker object."

The documentation is unfortunately brief and not really explaining what 
a ForwardingAliasSet is and why it is save to ignore. Looking at the 
uses, the AliasSetTracker internally seems to commonly skip 
ForwardingAliasSets, but e.g. neither the LoopVectorizer, the 
BBVectorizer nor the LoopRerollPass seem to use isForwardingSet() to 
filter out Forwarding sets. I got now some confidence that those sets 
are really save to ignore and that those passes just missed it the same 
way we missed it. If you did any similar investigation, I think it would 
have been useful to share it in the commit message.

>> Also, was there a reason you did not commit a test case? I attached you
>> the one I used, in case you did not manage to reduce one.
> The test case for this is so unstable, I couldn't reproduce it with an
> llvm from yesterday.

Interesting. It consistently failed on the LNT servers for several 
builds and I could also reproduce it on my machine. I wonder why it is 
not reproducible for you.

If you have a reason why you do/can not want to commit a certain test 
case, sharing this information in the commit message seems useful.

> The forwarding alias sets are also a pure artifact
> of the alias set tracker and for the last 11 years we are supposed to
> ignore them when iterating over an AST, I just didn't know.

Me neither and little people seem to know. Now we learned. It may make 
sense to hide this implementation detail in the AST.

Tobias