[PATCH] D71492: [SCEV] Generate AddRec for trivial and LCSSA phis outside of loop header (WIP)

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

In D71492#1784331 <https://reviews.llvm.org/D71492#1784331>, @Meinersbur wrote:

> [suggestion] The scope of the lcssa PHI node and the definition of its value are in different loops, hence calling `getSCEVAtScope` at the lcssa's scope may simplify the expression. This might even be mandatory since SCEV should be canonical.


If by "lcssa`s scope" you mean the scope at which the lcssa phi instruction appears, then it's not going to help in this case. The scope is the function (outside of any loop). If you pass nullptr to getSCEVAtScope it continues to produce SCEVUnknown.

> [serious] `createAddRecFromPHI` might be directly called be called `getSCEV` such that `%indvars.iv.next` isn't in the cache yet. Therefore the result changes depending on the order in which `getSCEV` is called.

Yes, I was originally concerned that calling getSCEV could cause an infinite loop for cyclic phis, but I gave it more thought and I couldn't come up with a valid IR that can cause the infinite loop. I've changed it to use getSCEV and documented my reasoning for why I don't think a cycle is possible.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5017
+      ValueExprMap.find_as(UniqueIncomingValue) != ValueExprMap.end()) {
+    const SCEV *Result = ValueExprMap[UniqueIncomingValue];
+    ValueExprMap[SCEVCallbackVH(PN, this)] = Result;
----------------
Meinersbur wrote:
> Did you consider using `getExistingSCEV` to check whether it already has been computed?
I've replaced this with a call to getSCEV(). Please also see my comment above.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5326
 const SCEV *ScalarEvolution::createNodeForPHI(PHINode *PN) {
   if (const SCEV *S = createAddRecFromPHI(PN))
     return S;
----------------
Meinersbur wrote:
> Why insering this into `createAddRecFromPHI` and not `createNodeForPHI`, its only caller? 
ok, I'll put it at the start of `createNodeForPHI`.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5332-5335
   // If the PHI has a single incoming value, follow that value, unless the
   // PHI's incoming blocks are in a different loop, in which case doing so
   // risks breaking LCSSA form. Instcombine would normally zap these, but
   // it doesn't have DominatorTree information, so it may miss cases.
----------------
fhahn wrote:
> Meinersbur wrote:
> > [serious] Shouldn't this code handle the case? It explicitly mentions the risk of breaking LCSSA, why isn't this the case for your code? 
> I think the problem here is that SCEVExpander does not preserve LCSSA at the moment while expanding expressions and looking through LCSSA would break LCSSA form while expanding SCEVs that would refer to a LCSSA instruction without looking through them.
> 
> Getting rid of that restriction was discussed in D68194 as well and I have been working on set of follow up patches that make SCEVExpander preserve LCSSA while expanding. I've put up the WIP patches at D71538 and D71539. There are still a few problems/refactors pending before they are ready though.
@fhahn are you considering the other types of non-trivial phis in your patches? eg. `%tmp24 = phi i64 [ %tmp14, %bb22 ], [ %tmp14, %bb13 ]` in the LIT test bellow.

I think this patch can complement D71539 assuming preservation of LCSSA by SCEVExpander goes in first. For now I can mark this as a child of your patch, unless you'd like to fit this change into yours to handle non-LCSSA trivial phis too.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71492/new/

https://reviews.llvm.org/D71492