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

[Michael Kruse via llvm-commits]

2015-08-17 15:15 GMT+02:00 Tobias Grosser <tobias at grosser.es>:
> On 08/14/2015 09:16 PM, Michael Kruse wrote:
>> 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.

Yes, I didn't see that.


>> 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.

I expected this would be rejected right away. I must be a reason why
this extension got implemented 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.

Is there some conceptual problem in ScalarEvolution what makes this impossible?

If this is the case, doesn't this mean we shouldn't use ScalarEvolution at all?


>> 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.

For http://reviews.llvm.org/D12053 I chose 6.)


>> 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".

The problem is that the sdiv/srem are categorized as loop-invariant
event though are are in the loop themselves. Categorized them as
loop-variant in SCEVValidator would get around this, but probably also
takes away its usefulness.


>> 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?

Right. I actually didn't know there the SCEVExpander is invoked within
the BlockGenerator, so I din't think thous could happen. There is a
function generateSCEV which for some reason is not used there.


>> 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.

Which number? They all are related to SCEVs.

I implemented 6.) as already mostly as workaround get some a "PHI
nodes in exit blocks" solution without lnt failures.

Michael