[PATCH] D14481: [SCEV] Simplify adds to a canonical sum of product

[Andrew Trick via llvm-commits]

atrick added a reviewer: mzolotukhin.
atrick added a comment.

Nice little SumOfProduct utility!

Exponential recursion needs to be prevented. Even without the simplification step, the visitor is exponential. Unlike normal SCEV construction, the recursive visitor is not memoizing results.

I guess the simplification is inherently exponential. I don't know how to avoid that without limitting size of the product. It does look like excess SCEVMulExprs may be created (for unit scale).

If you want to go forward with this, I would realy want to see motivating examples shown in the form of symbolic expressions. I couldn't understand how the utility worked without visualizing examples. It's not completely clear to my why this can't be done in place as a bottom-up SCEV canonicalization/simplification. I would need to work through some examples to understand the issue.

I guess the final question is wheter to use this as the canonical SCEV form rather an an on-the-side simplification. I would be fine with that if the Expander knew how to minimize multiplication by factoring terms.

Adding Michael Z. since I can't really do timely SCEV reviews.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1848
@@ +1847,3 @@
+class SumOfProduct {
+  DenseMap<const SCEV *, APInt> Terms;
+  unsigned BitWidth;
----------------
Did you consider SmallPtrSet?


http://reviews.llvm.org/D14481