On Tue, Jul 21, 2015 at 8:42 PM, Sebastian Pop <seb...@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.

+/* 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

Reply via email to