[PATCH] D34025: [SCEV] Teach SCEVExpander to expand BinPow
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 12 19:54:53 PDT 2017
mkazantsev added a comment.
>> This patch may still be a good idea, but for that you'll have to make a case by claiming that the new expanded form is a more canonical expansion of SCEVMulExpr s, not by claiming it is a bugfix. You'll also need to show that other passes don't get pessimized by such multiplication trees (unlikely).
Hi Sanjoy,
Actually an example of a better behavior is already present it test_03. If you look at it, you can notice that the new expander logic has changed the order of calculations. Maybe this example is not good enough because the overall number of multiplications remains the same, but if we have something like
x0 = a * a
x1 = x0 * a
x2 = x1 * a
...
xn = x(n-1) *a
then the new logic allows to reduce the overall number of multiplications from O(N) to O(log(N)). Would that be a sufficient justification of this change as improvement? I can add a test that does it.
As for limiting the operands merging to some limit - I am still not convinced that it is a good idea. What if we have the situation I described above, with n being like 1022? In fact, we have a chain of muls that calcupates a ^ 1024. It could come from a loop like:
x = a * a;
for (i = 0; i < 1022; i++)
x = x * a
Which got fully unrolled. In this case it is profitable to merge one operands in one SCEV, figure out that we actually need a power of 2 and then expand it into BinPow, decreasing the number of resulting muls from 1023 to ~10. If we set a merge limit, we can fail to do that.
Anyways, setting a merge limit for muls would be a separate patch. If you still do think we need it, I will make it. As for this one, if I add a test on what I described (when it reduces the overall number of muls), would that work to approve it?
Thanks!
https://reviews.llvm.org/D34025
More information about the llvm-commits
mailing list