[PATCH] D28158: [SCEV] limit recursion depth and operands number in getAddExpr
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 15 13:37:47 PST 2017
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
More information about the llvm-commits
mailing list