On Wed, Jul 22, 2015 at 1:01 PM, Richard Biener <richard.guent...@gmail.com> wrote: > 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.
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