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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D34025#777416, @mkazantsev wrote:

> Sanjoy, what is the difference between having one SCEV with exponential number of operands or having a SCEV tree with 2 operands in each node, but the same number of operands in total? We won't win anything doing so.


I don't think you'll need the same number of operands in total in the second case.  IIUC, the problematic IR is:

  x1 = x0 * x0;
  x2 = x1 * x1;
  x3 = x2 * x2;
  x4 = x3 * x3;

where the SCEV expression for `x4` ends up being `(x0 * x0 * ... 16 times)`.  This, in turn, gets lowered into:

  xa = x0 * x0;
  xb = xa * x0;
  ... 15 instructions

I'm postulating that the real problem was that we created a SCEV for `x4` with 16 operands.  If we instead had the SCEV for `x4` be:

  // Writing it this way since printing SCEVs obscures SCEV identity
  S0 = x0 * x0;
  S1 = S0 * S0;
  S2 = S1 * S1;
  S3 = S2 * S2;

then the expansion would naturally have been

  xa = x0 * x0;  // expand(S0)
  xb = xa * xa;  // expand(S1)
  xc = xb * xb;  // expand(S2)
  xd = xc * xc;  // expand(S3)

(Note that `expand` tries to re-use already expanded expressions, see `InsertedExpressions`).

Two caveats though:

1. Obviously, we don't want to prevent SCEV for merging multiplication-of-multiplication expressions completely -- some of that is required for good simplification.  We just want to do that up to a sane limit.
2. 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).


https://reviews.llvm.org/D34025