[PATCH] D79893: [SCEV] Constant fold MultExpr before applying depth limit.

[Max Kazantsev via Phabricator via llvm-commits]

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

I like the idea, however I would strongly avoid any situation where we can, even theoretically, recurse with more depth than limit.

Do you plan to make similar changes for other SCEV types (e.g. `Add`)?



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2960
       Ops[0] = getConstant(Fold);
+      if (Ops.size() == 2)
+        return Ops[0];
----------------
Is this change realy necessary? Looks unrelated, please commit it separately unless there is a reason to have it here. I'm also sure that similar change can be done in Add and some other SCEVs.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2982
             const SCEV *Mul = getMulExpr(Ops[0], AddOp, SCEV::FlagAnyWrap,
                                          Depth + 1);
             if (!isa<SCEVMulExpr>(Mul)) AnyFolded = true;
----------------
We can potentially recurse here with unlimited depth, which is undersiable. Can you please avoid this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79893