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

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

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? 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. Is one alias analysis using the results of another alias analysis something that works?

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?


Repository:
  rL LLVM

https://reviews.llvm.org/D44564