[polly] ScalarEvolution not supporting SDiv, SRem [was: r244903 - Add test case for SCEV synthesizing]

[Tobias Grosser via llvm-commits]

On 08/14/2015 09:16 PM, Michael Kruse wrote:
> There is a larger issue behind this test case that cannot be fixed easily.

Hi Michael,

thank you for looking into this.
  
> This particular test case is somewhat dump because %div44 = sdiv i64
> 0, 2 is constant. It could be optimized away using InstCombine. Before
> bugpoint it was not constant. About 6 programs from the test-suite
> (did not look into all of them, assuming because the same assert
> triggers) failed because of this when allowing PHI nodes in exit
> blocks.
>
> The cause is the following:
>
> As an exception, polly::SCEVValidator accepts SDiv and SRem
> instructions, for which no SCEV representation exists. They appear as
> SCEVUnknown. polly::SCEVAffinator has been taught how to generate an
> isl_pw_aff for them. When used as a scop parameter, llvm::SCEVExpander
> is used to generate IR code. It doesn't know about the SDiv/SRem
> extension and is happy to generate code referencing the SDiv/SRem that
> might be defined only later within the scop.

Right.
  
> AFAIK such uses of SCEVExpander can only occur for scop parameters,
> but nothing within the scop. Everything from within the scop is
> generated from the isl_ast/isl_pw_aff.

Actually, even within the scop we are using the SCEVExpander in
BlockGenerator::getNewValue so this problem may even reach out
until there.

> Possible solutions:
>
> 1.) Remove the extension for SDiv/SRem

Remove (or rather conditionally disable) this extension is is a valid
solution. It seems SDiv/SRem support is fundamentally incomplete. I
would be very happy if you could fix this, but it seems equally
fine to just disable it for now and go ahead with whatever
you planned to do in the first place.
  
> 2.) Teach ScalarEvolution handle SDiv and SRem, including the
> introduction of SCEVSDivExpr and SCEVSRemExpr. This requires changing
> LLVM itself.

Right, this needs LLVM changes. The other issue with this is, that I
believe we will at some point hit fundamental limitations of the
SCEVExpander. Specifically, I hope that we can at some point have the
SCEVValidator allow ternary conditions, min/max expressions, ...

Especially for ternary expressions I do not really see how we can
add them to SCEV and/or the SCEV expander. This really seems to
be out of scope.
  
> 3.) Copy&Paste LLVM's llvm::SCEVExpander and modify it such that it
> generates code for the SDiv and SRem instructions as well.

Copy pasting seems bad.

> 4.) Fix it up after SCEVExpander: Copy instructions/expression trees
> that are referenced in the generated code,(but defined later) to
> before the generated code. This possible because SCEVValidator checks
> that the operands (and their operands, recursively) are
> scop-invariant.

Right, that could work. I did not look into this in detail yet, but
this seems to be the approach you have taken so far. As for now,
there seems to exist at least a bug in the implementation.
  
> 5.) Demote SCEV's with SRem/SDiv instruction to at most IV, so they
> cannot appear in SCEVs outside of the loop. (Makes 4 test cases fail).

I do not understand this approach. What does it mean to demote to at most
"IV".

> 6.) Move scop-invariant SRem/SDivs to polly.split_new_and old before
> CodeGeneration.

Would this be a general solution? I assume this could fix the issues
we have currently test cases for, but probably not the ones where the
sdivs are loop-variant, no?

> Opinions? IMHO 2.) is the cleanest. 4.) and 6.) would be a quick fixups.

I must admit, I currently have a little bias to the SCEV based approach
I proposed in the patch review of 4), but may be convinced that some
other solution might be better.

Best,
Tobias