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

Sebastian Pop via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 22 09:32:47 PDT 2018


sebpop added a comment.

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

> After some tinkering I've come up with the following solution (I have something that seems to work, but it needs cleaning up and testing):
>
> - Add a PhiAnalysis analysis pass which returns a PhiInfo


I think this makes sense as Danny suggested to move everything that is not constant time
away from BasicAA in different analysis passes whose results could be cached and invalidated.

> - PhiInfo::getUnderlyingPhiValues takes a PHINode and returns the set of non-phi values reachable from that phi

I think this makes sense as it goes in the same direction where Danny was pointing earlier:
"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."

> - PhiInfo stores a map of PHINode->values which is initially empty, but is incrementally updated whenever getUnderlyingPhiValues is called on a phi we haven't yet calculated
> - BasicAA uses PhiAnalysis if it's available
> - When it is BasicAAResults::aliasPHI populates V1Srcs from getUnderlyingPhiValues, otherwise it does what it currently does
> - MemoryDependenceAnalysis uses PhiAnalysis, which then makes it available to BasicAA
> - MemoryDependenceResults::removeInstruction clears the PhiInfo map, to make sure later queries are correct
> 
>   Doing all this gets me what I want, which is GVN getting correct aliasing information for the phis that it inserts. I've avoided doing this by adding another alias analysis to basically limit how much work I need to do to get this working, and it means whoever does do that in the future can just make the new alias analysis use PhiAnalysis for that part of it. Also there's maybe other parts of the compiler that want this kind of information.
> 
>   Does this seem reasonable?

I think this proposal is reasonable, although Danny should comment on it.
In the mean time, could you please post the updated patch? Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D44564





More information about the llvm-commits mailing list