[PATCH] D44564: [BasicAA] Relax restriction on PHI node handling.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D44564#1079811, @john.brawn wrote:

> In https://reviews.llvm.org/D44564#1078564, @efriedma wrote:
>
> > Hmm.  I don't see anything wrong with the code, but how often will this trigger?  instcombine will remove the PHI nodes in your testcase, so this is only a problem if you have a pass which creates PHI cycles like this.  I guess GVN does in some cases?  Can we avoid doing that?
>
>
> Yes, this comes from GVN where it temporarily introduces phi nodes like this that are later removed, but which in the meantime prevent PRE from happening because of this inaccurate alias analysis. I'd previously gone for the approach of fixing this in GVN in https://reviews.llvm.org/D44160, but the conclusion there was to do it in alias analysis.
>
> In https://reviews.llvm.org/D44564#1078585, @dberlin wrote:
>
> > Yeah, i'd really like to understand the value of this before make this code more complex and handle more cases.
> >
> > Really, as we've discussed many times, BasicAA's complex phi/cycle handling should probably be moved out to a caching forward pass that properly resolves cycles, instead of a lazy backwards pass that tries depth limiting to handle cycles.  This was waiting on the new pass manager work, AFAIK (because you couldn't cache it otherwise)
> >
> > Now that this is done (again, AFAIK), one way to start doing that would be to make PhiAA here, have it handle the cycles you are talking about (it's trivial to compute phi-induced cycles and it's not going to affect compile time in any meaningful way even if you invalidate it a lot. There is code to do it already if you want it, it's <100 lines)  and do caching, and fall back to BasicAA.
> >
> > Then we can slowly move functionality into that, from BasicAA.
>
>
> If I understand you correctly, you're suggesting adding a new alias analysis pass which just handles phis (but by analyising them forwards, instead of backwards as BasicAA does), yes?


Yes.

> This alias analysis would have to make use of the results of other alias analyses though: what we're doing is comparing a phi P against a value V by finding the underlying values of P then calling aliasCheck for those values and V.

FWIW: What we do is actually not great. 
What you really *want* to do is build the SCC's formed solely by the phi nodes so you collapse any cycles and only have to look at the operands that aren't phis.
You can see an example of this kind of algorithm in the scc based phi minimization here:
https://pp.info.uni-karlsruhe.de/uploads/publikationen/braun13cc.pdf

Then, somewhat similar to what you see driving that, the outerops is the set of stuff to check what the phi can alias.

> Is one alias analysis using the results of another alias analysis something that works?

Yes, this works fine, AFAIK. We definitely do it right now :)

> Actually thinking about this while writing the above, it would probably make more sense to do things the other way around: have an analysis which gets the underlying values for a phi, and have BasicAA use that. So instead of building up V1Srcs here, we would instead do something like V1Srcs = PhiAnalysis.getUnderlyingValues(PN). I think that would be better?

This is actually not possible, but that isn't going to be obvious to someone who hasn't delved into this.

Let me also try to back up and give a little context and see if you still come to that answer:
The way it works right now is that the  AA code walks through a hierarchy of AA analysis, and keeps asking each until it gets NoAlias or MustAlias.
BasicAA is one of those passes.

Because of <technical reasons not worth getting into>, it used to be hard to build another AA that reused results from another analysis, and so people shoved more and more complex stuff stuff in BasicAA :)

Unfortunately, BasicAA is meant to be stateless and explicitly does no caching, and so these expensive computations occur every time you ask it an aliasing question.
You can't really have something it uses do caching, because there is no way to "invalidate" BasicAA.
The long term goal is to move those expensive computations somewhere else where they can be cached, and where the AA can be invalidated.

The personal way i viewed it was you have something like:

BasicAA <- gives simple, stateless AA answers that are ideally, constant time.  
<One or more CachingAA depending on what we discover as we do this > <- gives the answers that are currently linear time or worse.

Right now, because BasicAA is so slow, it's at the *bottom* and not *the top*. We  actually ask everything *before* we ask BasicAA, because it's N^2 (excepting depth limiting trying to keep it linear).

In this new world orders, BasicAA is at the top, doing quick checks, and checks get more expensive as you get down (similar to dependence analysis)


Repository:
  rL LLVM

https://reviews.llvm.org/D44564