[PATCH] [Polly] Added support for modulo expressions

[Tobias Grosser]

On 24.01.2015 18:23, Johannes Doerfert wrote:
> I added some comments and will change than patch accordingly. I will
> also split two smaller parts and remove some test cases for which I
> unfortunately can't remember the exact reason I added them. Anyway,
> thanks for the review.

You are welcome and sorry again for the delay.

> ================
> Comment at: lib/Analysis/ScopInfo.cpp:326
> @@ +325,3 @@
> +           "Modulo expressions are only valid for a constant right hand side");
> +    RHSVal = isl_aff_get_constant_val(RHSAff);
> +
> ----------------
> grosser wrote:
>> This is a lot of code, just to get a constant. Assuming we run an
>> instruction simplification pass before Polly, can it really happen
>> that RHS is not an explicit constant? If we assume the RHS is always
>> a constant, we could simplify this by directly converting
>> BO->getOperand(1) to an isl_pw_aff.
>>
>> Also, as previously discussed, the use of isl's aff_mod_val() is
>> incorrect for cases where the LHS is negative, as the semantics of
>> isl_aff_mod and srem differ.
> The second concern is important and I will correct that. Regarding the
> first, I'm not sure, maybe if we check this in the ScopDetection we
> can just assume a constant RHS here. I'm not sure which is better and
> do not have any arguments for one or another, so we should just choose
> one. (Or we do a isa<ConstantInt>(BO->getOperand(1)) here?)

I like the isa<ConstantInt> approach. Maybe we can use it in the
SCEVValidator?

> ================
> Comment at: lib/Support/SCEVValidator.cpp:386
> @@ +385,3 @@
> +        if (!Op0.isValid())
> +          return ValidatorResult(SCEVType::INVALID);
> +
> ----------------
> grosser wrote:
>> It would be nice to add a corresponding DEBUG statement to this
>> INVALID instruction, which explains why the statement is refused
>> (this is done consistently in this class).
> I can change this to return Op0 if to make it clear that the DEBUG
> message was already produced in the recursive step.
> Or did I miss something?

This was just by observation. If it gives a reasonable debug output,
then the code is fine.

> ================
> Comment at: test/ScopInfo/NonAffine/non_affine_but_modulo_condition_8.ll:14
> @@ +13,3 @@
> +; CHECK:     Scattering :=
> +; CHECK:         { Stmt_if_then[i0] -> scattering[0, i0, 0] };
> +; CHECK:     ReadAccess := [Reduction Type: +]
> ----------------
> grosser wrote:
>> The test cases fail ATM for me, because of the additional '0's in the scattering
> I'll change it ...

Thank you and sorry for adding this burdon on you purely due to me being
slow in reviewing.

Cheers,
Tobias