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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

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

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



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5017
+      ValueExprMap.find_as(UniqueIncomingValue) != ValueExprMap.end()) {
+    const SCEV *Result = ValueExprMap[UniqueIncomingValue];
+    ValueExprMap[SCEVCallbackVH(PN, this)] = Result;
----------------
Did you consider using `getExistingSCEV` to check whether it already has been computed?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5326
 const SCEV *ScalarEvolution::createNodeForPHI(PHINode *PN) {
   if (const SCEV *S = createAddRecFromPHI(PN))
     return S;
----------------
Why insering this into `createAddRecFromPHI` and not `createNodeForPHI`, its only caller? 


================
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.
----------------
[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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71492