This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC, PR66873] Use graphite for parloops
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Sebastian Pop <sebpop at gmail dot com>
- Cc: Tom de Vries <Tom_deVries at mentor dot com>, "gcc-patches at gnu dot org" <gcc-patches at gnu dot org>
- Date: Wed, 22 Jul 2015 13:02:22 +0200
- Subject: Re: [RFC, PR66873] Use graphite for parloops
- Authentication-results: sourceware.org; auth=none
- References: <55A6C1DF dot 1050108 at mentor dot com> <20150720183141 dot GB20717 at f1 dot c dot bardezibar dot internal> <55AD9093 dot 1060206 at mentor dot com> <55AE5340 dot 2010700 at mentor dot com> <20150721184249 dot GA7417 at f1 dot c dot bardezibar dot internal> <CAFiYyc1Rqzq4BCRWqcVvWMEGRO3CaABWMDayXSXssF3oWrXhUA at mail dot gmail dot com>
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