[PATCH] D65276: [SCEV] Disable canonical expansion for nested recurrences.

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko added a comment.

The original patch introduces a function `minIterationWidthForEvaluateAtIteration`. This way it exposes the limitation of evaluateAtIteration through the API and makes it possible for the user to choose the proper It width (e.g. like I did in the first revision https://reviews.llvm.org/D62563?vs=on&id=201795). What was the reason to remove it?



================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:347-349
+    /// Note that SCEVCouldNotCompute is returned if evaluation attempt failed.
+    /// For example, if precision of \p It is less than required to perform an
+    /// evaluation instance of SCEVCouldNotCompute is returned.
----------------
Because you removed `minIterationWidthForEvaluateAtIteration` from the API, now the description on `evaluateAtIteration` is very vague. I'd prefer to specify this limitation more specifically.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1208-1212
+  // Zero extension indicates insufficient precision of incoming "It".
+  // In this case result of calculations may be incorrect. Go ahead and return
+  // SCEVCouldNotCompute.
+  if (isa<SCEVZeroExtendExpr>(Dividend))
+    return SE.getCouldNotCompute();
----------------
Any reason to express this check in this way and not as a comparison of CalculationBits with It bitwidth?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:11670
   case scCouldNotCompute:
-    llvm_unreachable("Attempt to use a SCEVCouldNotCompute object!");
+    return LoopVariant;
   }
----------------
I don't think that it makes much sense to ask loop disposition of scCouldNotCompute. I think the caller of this API should handle scCouldNotCompute case instead. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65276