[RFC,PR66873] Use graphite for parloops

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?  Please use FLOAT_TYPE_P.

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

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.  Please instead XFAIL the existing ones and add variants
with unsigned
reductions.  Adding -fwrapv isn't a good solution either.

Can you think of a testcase that breaks btw?

The "proper" solution (see other passes) is to rewrite the reduction
to a wrapping
one (cast to unsigned for the reduction op).

+  return (FIXED_POINT_TYPE_P (type)
+         && FIXED_POINT_TYPE_OVERFLOW_WRAPS_P (type));

why?  Simply return false here instead?

I suggest to
leave fixed-point changes out from the initial patch submission.

Thanks,
Richard.

> Thanks,
> Sebastian

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?  Please use FLOAT_TYPE_P.
>
> +  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.
>
> 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.  Please instead XFAIL the existing ones and add variants
> with unsigned
> reductions.  Adding -fwrapv isn't a good solution either.
>
> Can you think of a testcase that breaks btw?
>
> The "proper" solution (see other passes) is to rewrite the reduction
> to a wrapping
> one (cast to unsigned for the reduction op).
>
> +  return (FIXED_POINT_TYPE_P (type)
> +         && FIXED_POINT_TYPE_OVERFLOW_WRAPS_P (type));
>
> why?  Simply return false here instead?
>
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 9145dbf..e014be2 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -2613,16 +2613,30 @@ vect_is_simple_reduction_1 (loop_vec_info
> loop_info, gimple phi,
>                         "reduction: unsafe fp math optimization: ");
>        return NULL;
>      }
> -  else if (INTEGRAL_TYPE_P (type) && TYPE_OVERFLOW_TRAPS (type)
> -          && check_reduction)
> +  else if (INTEGRAL_TYPE_P (type) && check_reduction)
>      {
> ...
>
> You didn't need to adjust any testcases?  That's probably because the
> checking above is
> not always executed (see PR66623 for a related testcase).  The code
> needs refactoring.
> And we need a way-out, that is, we do _not_ want to not vectorize
> signed reductions.
> So you need to fix code generation instead.

Btw, for the vectorizer the current "trick" is that nobody takes advantage about
overflow undefinedness for vector types.

> +/* Nonzero if fixed-point type TYPE wraps at overflow.
> +
> +   GCC support of fixed-point types as specified by the draft technical report
> +   (N1169 draft of ISO/IEC DTR 18037) is incomplete: Pragmas to
> control overflow
> +   and rounding behaviors are not implemented.
> +
> +   So, if not saturating, we assume modular wrap-around (see Annex E.4 Modwrap
> +   overflow).  */
> +
> +#define FIXED_POINT_TYPE_OVERFLOW_WRAPS_P(TYPE) \
> +  (NON_SAT_FIXED_POINT_TYPE_P (TYPE))
>
> somebody with knowledge about fixed-point types needs to review this.
> I suggest to
> leave fixed-point changes out from the initial patch submission.
>
> Thanks,
> Richard.
>
>> Thanks,
>> Sebastian