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

Thanks,
- Tom