[PATCH] D35990: [SCEV] Prohibit SCEV transformations for huge SCEVs

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

You need some tests highlighting the limit working in practice.  These can be easily written by using non-default threshold values.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:858
 
+static bool isHugeExpression(const SCEV *S) {
+  return S->getExpressionSize() >= HugeExprThreshold;
----------------
Please add a comment here explaining what "huge" means and is used for.  This the obvious point for someone reading the code to try to find the explanation. 


================
Comment at: lib/Analysis/ScalarEvolution.cpp:862
+
+static bool hasHugeExpression(SmallVectorImpl<const SCEV *> &Ops) {
+  return any_of(Ops, isHugeExpression);
----------------
Probably better to use an ArrayRef as the argument.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2909
   unsigned Idx = 0;
   if (const SCEVConstant *LHSC = dyn_cast<SCEVConstant>(Ops[0])) {
 
----------------
Not related to your review, but I notice inconsistency about whether we merge adjacent constants before or after the bailout check.


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

https://reviews.llvm.org/D35990