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

[Johannes Doerfert]

On 10/08, Tobias Grosser wrote:
> 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."
Brief indeed, but it seems clear to me that ignoring those sets is the
right way to go.

> 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.
I think they missed it as we did too. Maybe we should tell them ;)

> >>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.
Within the llvm changes of one day I could reproduce it but not before,
I don't know why and I don't understand the AST good enough to guess.

> 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.
I missed a test case and to properly explain the change, if you want you
can rectify my mistake and add a test case that might introduce
forwarding sets.

> >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.
True.

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.26

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141008/7c4215a4/attachment.sig>