[PATCH] D34025: [SCEV] Teach SCEVExpander to expand BinPow

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Daniel, regarding getAddExpr: SCEV tends to keep its expressions as flat as possible, so if it has two (or more) equal args in SCEVAddExpr, it transforms it into multiplication. You can try to invoke getAddExpr for (a, a, a) and it will return simething like (3 * a). So transforming Adds to Muls is not a problem, the problem with Muls is that we don't have a Power construct in SCEV that would be exactly what we need to represent such expressions. But as I said in the comment, we don't have it and it wouldn't be easy to introduce it.



================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:766
+    // 1u << 31 at most to not deal with unsigned overflow.
+    while (E != OpsAndLoops.end() && *I == *E && Exponent != MaxExponent) {
+      ++Exponent; ++E;
----------------
dneilson wrote:
> How challenging would it be to enhance SCEVMulExpr to represent repeated multiplies by the same value as a pair (value, exponent) rather than as a list of the same value being repeated 'exponent' times?
> 
> This patch has addressed not having to expand a ^ (2 ^ N) at runtime, but we still have an exponential number of terms in the OpsAndLoops list, which should quite negatively affect compile time.
I agree with your concern. In fact what you propose is to introduce a new construct like SCEVPower. It would be extremely nice to have, but unfortunately it would be a really huge change affecting all SCEV.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:773
+    // that are needed into the result.
+    Value *P = expandCodeFor(I->second, Ty);
+    Value *Result = nullptr;
----------------
dneilson wrote:
> If Ty is a float type and N isn't a trivial value (like 1 or 2) then it can be better (faster & more accurate) to generate a call to @llvm.powi rather than the tree of multiplies.
SCEV doesn't work with FP types, so it is not our case. :) But if it did, such transformations changing the order of calculations could not even be legal at all.


https://reviews.llvm.org/D34025