[PATCH] D28158: [SCEV] limit recursion depth and operands number in getAddExpr

[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: include/llvm/Analysis/ScalarEvolution.h:1158
+  const SCEV *getAddExpr(const SCEV *LHS, const SCEV *RHS,
+                         unsigned Depth) {
+    SmallVector<const SCEV *, 2> Ops = {LHS, RHS};
----------------
Given that the "recursive" calls are really just tail calls, perhaps we can call this `Iteration` instead of `Depth`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2143
 
-  // Okay, check to see if the same value occurs in the operand list more than
-  // once.  If so, merge them together into an multiply expression.  Since we
-  // sorted the list, these values are required to be adjacent.
-  Type *Ty = Ops[0]->getType();
-  bool FoundMatch = false;
-  for (unsigned i = 0, e = Ops.size(); i != e-1; ++i)
-    if (Ops[i] == Ops[i+1]) {      //  X + Y + Y  -->  X + Y*2
-      // Scan ahead to count how many equal operands there are.
-      unsigned Count = 2;
-      while (i+Count != e && Ops[i+Count] == Ops[i])
-        ++Count;
-      // Merge the values into a multiply.
-      const SCEV *Scale = getConstant(Ty, Count);
-      const SCEV *Mul = getMulExpr(Scale, Ops[i]);
-      if (Ops.size() == Count)
-        return Mul;
-      Ops[i] = Mul;
-      Ops.erase(Ops.begin()+i+1, Ops.begin()+i+Count);
-      --i; e -= Count - 1;
-      FoundMatch = true;
-    }
-  if (FoundMatch)
-    return getAddExpr(Ops, Flags);
-
-  // Check for truncates. If all the operands are truncated from the same
-  // type, see if factoring out the truncate would permit the result to be
-  // folded. eg., trunc(x) + m*trunc(n) --> trunc(x + trunc(m)*n)
-  // if the contents of the resulting outer trunc fold to something simple.
-  for (; Idx < Ops.size() && isa<SCEVTruncateExpr>(Ops[Idx]); ++Idx) {
-    const SCEVTruncateExpr *Trunc = cast<SCEVTruncateExpr>(Ops[Idx]);
-    Type *DstType = Trunc->getType();
-    Type *SrcType = Trunc->getOperand()->getType();
-    SmallVector<const SCEV *, 8> LargeOps;
-    bool Ok = true;
-    // Check all the operands to see if they can be represented in the
-    // source type of the truncate.
-    for (unsigned i = 0, e = Ops.size(); i != e; ++i) {
-      if (const SCEVTruncateExpr *T = dyn_cast<SCEVTruncateExpr>(Ops[i])) {
-        if (T->getOperand()->getType() != SrcType) {
-          Ok = false;
-          break;
-        }
-        LargeOps.push_back(T->getOperand());
-      } else if (const SCEVConstant *C = dyn_cast<SCEVConstant>(Ops[i])) {
-        LargeOps.push_back(getAnyExtendExpr(C, SrcType));
-      } else if (const SCEVMulExpr *M = dyn_cast<SCEVMulExpr>(Ops[i])) {
-        SmallVector<const SCEV *, 8> LargeMulOps;
-        for (unsigned j = 0, f = M->getNumOperands(); j != f && Ok; ++j) {
-          if (const SCEVTruncateExpr *T =
-                dyn_cast<SCEVTruncateExpr>(M->getOperand(j))) {
-            if (T->getOperand()->getType() != SrcType) {
+  if (Depth <= MaxAddExprDepth) {
+    // Okay, check to see if the same value occurs in the operand list more than
----------------
Use early return here instead.  I.e. extract out the actual node construction logic into a helper function, and call that here on an early return.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:2231
+        if (Ops.size() > AddOpsInlineThreshold ||
+            Add->getNumOperands() > AddOpsInlineThreshold)
+          break;
----------------
Can you please split out this bailout into a separate change with its own test?


================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:537
+  FunctionType *FTy =
+      FunctionType::get(Type::getVoidTy(Context), std::vector<Type *>(), false);
+  Function *F = cast<Function>(M.getOrInsertFunction("f", FTy));
----------------
s/`std::vector<Type *>()`/`{}`/


================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:541
+
+  auto *Ty = Type::getInt32Ty(Context);
+  Instruction *Mul1 = BinaryOperator::CreateMul(UndefValue::get(Ty), UndefValue::get(Ty), "", EntryBB);
----------------
clang-format this please.


================
Comment at: unittests/Analysis/ScalarEvolutionTest.cpp:543
+  Instruction *Mul1 = BinaryOperator::CreateMul(UndefValue::get(Ty), UndefValue::get(Ty), "", EntryBB);
+  Instruction *Add1 = BinaryOperator::CreateAdd(Mul1, UndefValue::get(Ty), "", EntryBB);
+  Mul1 = BinaryOperator::CreateMul(Add1, UndefValue::get(Ty), "", EntryBB);
----------------
Can you please use a real value (an `llvm::Argument`, say) instead of `undef`?  SCEV does not simplify `undef` today, but that can change in the future, rendering this test a no-op.


https://reviews.llvm.org/D28158