Fix PR25372 - teach replaceCongruentPHIs to handle cases where SE evaluates a PHI to a SCEVConstant

[sbaranga]

Summary

Since now Scalar Evolution can create non-add rec expressions for PHI
nodes, it can also create SCEVConstant expressions. This will confuse
replaceCongruentPHIs, which previously relied on the fact that SCEV
could not produce constants in this case.

We will now replace the node with a constant in these cases - or avoid
processing the Phi in case of a type mismatch.

[sanjoy]

lgtm with comments addressed

lib/Analysis/ScalarEvolutionExpander.cpp

1740 ↗

(On Diff #38903)

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)`

1742 ↗

(On Diff #38903)

Instead of having a standalone decl for Const, why not

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

[sbaranga]

Added a lambda to handle all PHI simplifications (which should address all comments).

[sbaranga]

Committed in r251938, thanks!

-Silviu

llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp

unsigned SCEVExpander::replaceCongruentIVs(Loop *L, const DominatorTree *DT,
   unsigned NumElim = 0;
   DenseMap<const SCEV *, PHINode *> ExprToIVMap;
   // Process phis from wide to narrow. Map wide phis to their truncation
   // so narrow phis can reuse them.
   for (SmallVectorImpl<PHINode*>::const_iterator PIter = Phis.begin(),
          PEnd = Phis.end(); PIter != PEnd; ++PIter) {
     PHINode *Phi = *PIter;
 
+    auto SimplifyPHINode = [&](PHINode *PN) -> Value * {
+      if (Value *V = SimplifyInstruction(PN, DL, &SE.TLI, &SE.DT, &SE.AC))
+        return V;
+      if (!SE.isSCEVable(PN->getType()))
+        return nullptr;
+      auto *Const = dyn_cast<SCEVConstant>(SE.getSCEV(PN));
+      if (!Const)
+        return nullptr;
+      return Const->getValue();
+    };
+
     // Fold constant phis. They may be congruent to other constant phis and
     // would confuse the logic below that expects proper IVs.
-    if (Value *V = SimplifyInstruction(Phi, DL, &SE.TLI, &SE.DT, &SE.AC)) {
+    if (Value *V = SimplifyPHINode(Phi)) {
+      if (V->getType() != Phi->getType())
+        continue;
       Phi->replaceAllUsesWith(V);
       DeadInsts.emplace_back(Phi);
       ++NumElim;
       DEBUG_WITH_TYPE(DebugType, dbgs()
                       << "INDVARS: Eliminated constant iv: " << *Phi << '\n');
       continue;
     }
 

llvm/trunk/test/Transforms/IndVarSimplify/const_phi.ll

+; RUN: opt < %s -indvars -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; PR25372
+; We can compute the expression of %phi0 and that is a SCEV
+; constant. However, instcombine can't deduce this, so we can
+; potentially end up trying to handle a constant when replacing
+; congruent IVs.
+
+; CHECK-LABEL: crash
+define void @crash() {
+entry:
+  br i1 false, label %not_taken, label %pre
+
+not_taken:
+  br label %pre
+
+pre:
+; %phi0.pre and %phi1.pre are evaluated by SCEV to constant 0.
+  %phi0.pre = phi i32 [ 0, %entry ], [ 2, %not_taken ]
+  %phi1.pre = phi i32 [ 0, %entry ], [ 1, %not_taken ]
+  br label %loop
+
+loop:
+; %phi0 and %phi1 are evaluated by SCEV to constant 0.
+  %phi0 = phi i32 [ 0, %loop ], [ %phi0.pre, %pre ]
+  %phi1 = phi i32 [ 0, %loop ], [ %phi1.pre, %pre ]
+  br i1 undef, label %exit, label %loop
+
+exit:
+  ret void
+}