[PATCH] D32663: [SCEV] createAddRecFromPHI: Optimize for the most common case.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4092
+/// technique for finding the AddRec expression.
+const SCEV *ScalarEvolution::createAddRecFromPHISimpleCase(PHINode *PN,
+                                                           Value *BEValueV,
----------------
How about `createSimpleAffineAddRec`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4097
+  assert(L && L->getHeader() == PN->getParent());
+  assert (BEValueV && StartValueV);
+
----------------
Bad space?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4108
+  if (BO->LHS == PN)
+    Accum = getExistingSCEV(BO->RHS);
+  else if (BO->RHS == PN)
----------------
I'd avoid using `getExistingSCEV` here.  Instead, how about checking `L->isLoopInvariant(BO->RHS or BO->LHS)` and if so, call `getSCEV`?  Then you won't waste work since if `L->isLoopInvariant(BO->RHS or BO->LHS)` is true then you know you're going to be able to return an add recurrence.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:4109
+    Accum = getExistingSCEV(BO->RHS);
+  else if (BO->RHS == PN)
+    Accum = getExistingSCEV(BO->LHS);
----------------
As discussed on IRC, please check if this is actually viable.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4118
+  if (!isLoopInvariant(Accum, L) &&
+      (!isa<SCEVAddRecExpr>(Accum) ||
+       cast<SCEVAddRecExpr>(Accum)->getLoop() != L))
----------------
If you do the check above on `L->isLoopInvariant(BO->RHS or BO->LHS)` then this won't be necessary.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4176
+  // value for PN.
+  if (auto S = createAddRecFromPHISimpleCase(PN, BEValueV, StartValueV))
+    return S;
----------------
`auto *`.


https://reviews.llvm.org/D32663