On Wed, Jul 22, 2015 at 6:00 PM, Tom de Vries <tom_devr...@mentor.com> wrote:
> [ 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.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?
>
>
> 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?
Ok if Sebastian is fine with it.
Richard.
> Thanks,
> - Tom
>
