[PATCH] D14230: Fix PR25372 - teach replaceCongruentPHIs to handle cases where SE evaluates a PHI to a SCEVConstant

[Sanjoy Das via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with comments addressed


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1740
@@ -1739,2 +1739,3 @@
     // would confuse the logic below that expects proper IVs.
-    if (Value *V = SimplifyInstruction(Phi, DL, &SE.TLI, &SE.DT, &SE.AC)) {
+    Value *V = SimplifyInstruction(Phi, DL, &SE.TLI, &SE.DT, &SE.AC);
+    if (!V) {
----------------
This is optional to fix -- it might be clearer to restructure this as

```
auto SimplifyPHINode = [&](PHINode *PN) {
  // see if SimplifyInstruction or SE is able to simplify PN, if so return the simplified Value* else return nullptr
}

if (Value *V = SimplifyPHIToConstant(Phi))
  // the logic that is currently guarded by `if (V)`
```


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1742
@@ +1741,3 @@
+    if (!V) {
+      const SCEVConstant *Const = nullptr;
+      if (SE.isSCEVable(Phi->getType())) {
----------------
Instead of having a standalone decl for `Const`, why not

```
if (SE.isSCEVable ...) {
 if (auto *SC = dyn_cast<SCEVConstant>(SE.getSCEV(...
   // use SC
```



http://reviews.llvm.org/D14230