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

[Max Kazantsev via Phabricator via llvm-commits]

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