Re: [PATCH] Don't allow unsafe reductions in graphite

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jul 22, 2015 at 6:00 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> [ was: Re: [RFC, PR66873] Use graphite for parloops ]
>
> On 22/07/15 13:02, Richard Biener wrote:
>>
>> On Wed, Jul 22, 2015 at 1:01 PM, Richard Biener
>> <richard.guenther@gmail.com>  wrote:
>>>
>>> >On Tue, Jul 21, 2015 at 8:42 PM, Sebastian Pop<sebpop@gmail.com>  wrote:
>>>>
>>>> >>Tom de Vries wrote:
>>>>>
>>>>> >>>Fix reduction safety checks
>>>>> >>>
>>>>> >>>       * graphite-sese-to-poly.c (is_reduction_operation_p): Limit
>>>>> >>>       flag_associative_math to SCALAR_FLOAT_TYPE_P.  Honour
>>>>> >>>       TYPE_OVERFLOW_TRAPS and TYPE_OVERFLOW_WRAPS for
>>>>> >>> INTEGRAL_TYPE_P.
>>>>> >>>       Only allow wrapping fixed-point otherwise.
>>>>> >>>       (build_poly_scop): Always call
>>>>> >>>       rewrite_commutative_reductions_out_of_ssa.
>>>>
>>>> >>
>>>> >>The changes to graphite look good to me.
>>>
>>> >
>>> >+  if (SCALAR_FLOAT_TYPE_P (type))
>>> >+    return flag_associative_math;
>>> >+
>>> >
>>> >why only scalar floats?
>
>
> Copied from the conditions in vect_is_simple_reduction_1.
>
>>> >Please use FLOAT_TYPE_P.
>
> Done.
>
>>> >
>>> >+  if (INTEGRAL_TYPE_P (type))
>>> >+    return (!TYPE_OVERFLOW_TRAPS (type)
>>> >+           && TYPE_OVERFLOW_WRAPS (type));
>>> >
>>> >it cannot both wrap and trap thus TYPE_OVERFLOW_WRAPS is enough.
>>> >
>
>
> Done.
>
>>> >I'm sure you'll disable quite some parallelization this way... (the
>>> >routine is modeled after
>>> >the vectorizers IIRC, so it would be affected as well).  Yeah - I see
>>> >you modify autopar
>>> >testcases.
>
>
> I now split up the patch, this bit only relates to graphite, so no autopar
> testcases are affected.
>
>>> >Please instead XFAIL the existing ones and add variants
>>> >with unsigned
>>> >reductions.  Adding -fwrapv isn't a good solution either.
>
>
> Done.
>
>>> >
>>> >Can you think of a testcase that breaks btw?
>>> >
>
>
> If you mean a testcase that fails to execute properly with the fix, and
> executes correctly with the fix, then no.  The problem this patch is trying
> to fix, is that we assume wrapping overflow without fwrapv. In order to run
> into a runtime failure, we need a target that does not do wrapping overflow
> without fwrapv.
>
>>> >The "proper" solution (see other passes) is to rewrite the reduction
>>> >to a wrapping
>>> >one (cast to unsigned for the reduction op).
>>> >
>
>
> Right.
>
>>> >+  return (FIXED_POINT_TYPE_P (type)
>>> >+         && FIXED_POINT_TYPE_OVERFLOW_WRAPS_P (type));
>>> >
>>> >why?
>
>
> Again, copied from the conditions in vect_is_simple_reduction_1.
>
>>> >  Simply return false here instead?
>
> Done.
>
>
> [ Btw, looking at associative_tree_code, I realized that the
>   overflow checking is only necessary for PLUS_EXPR and MULT_EXPR:
> ...
>   switch (code)
>     {
>     case BIT_IOR_EXPR:
>     case BIT_AND_EXPR:
>     case BIT_XOR_EXPR:
>     case PLUS_EXPR:
>     case MULT_EXPR:
>     case MIN_EXPR:
>     case MAX_EXPR:
>       return true;
> ...
>
> The other operators cannot overflow to begin with. My guess is that it's
> better to leave this for a trunk-only follow-up patch.
> ]
>
> Currently bootstrapping and reg-testing on x86_64.
>
> OK for trunk?
>
> OK 5 and 4.9 release branches?

Ok if Sebastian is fine with it.

Richard.

> Thanks,
> - Tom
>