[PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

Hi all,

This patch fixes the PR in question which is a miscompilation of gcc.dg/fixed-point/unary.c on arm.
It just restricts the A - B -> A + (-B) transformation when the type is fixed-point.

This fixes the testcase for me.
Is this the right approach?

Bootstrap and test on arm and x86 running.

Ok if testing is clean?

Thanks,
Kyrill


2015-07-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR middle-end/66915
     * match.pd (A - B -> A + (-B)): Don't allow folding
     when type if a fixed-point type.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: matchpd-fixed-point.patch --]
[-- Type: text/x-patch; name=matchpd-fixed-point.patch, Size: 692 bytes --]

commit c6669b5cde3d7b504aec388282e7af955af58681
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Jul 20 15:02:17 2015 +0100

    [match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

diff --git a/gcc/match.pd b/gcc/match.pd
index 4427000..3d7b32e 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -522,8 +522,8 @@ along with GCC; see the file COPYING3.  If not see
 /* A - B -> A + (-B) if B is easily negatable.  */
 (simplify
  (minus @0 negate_expr_p@1)
- (plus @0 (negate @1)))
-
+ (if (!FIXED_POINT_TYPE_P (type))
+ (plus @0 (negate @1))))
 
 /* Try to fold (type) X op CST -> (type) (X op ((type-x) CST))
    when profitable.

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Richard Biener]

On Mon, 20 Jul 2015, Kyrill Tkachov wrote:

> Hi all,
> 
> This patch fixes the PR in question which is a miscompilation of
> gcc.dg/fixed-point/unary.c on arm.
> It just restricts the A - B -> A + (-B) transformation when the type is
> fixed-point.
> 
> This fixes the testcase for me.
> Is this the right approach?
> 
> Bootstrap and test on arm and x86 running.
> 
> Ok if testing is clean?

Ok, but I think the fold-const.c code has the same issue, no:

      /* A - B -> A + (-B) if B is easily negatable.  */
      if (negate_expr_p (arg1)
          && !TYPE_OVERFLOW_SANITIZED (type)
          && ((FLOAT_TYPE_P (type)
               /* Avoid this transformation if B is a positive REAL_CST.  
*/
               && (TREE_CODE (arg1) != REAL_CST
                   ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
              || INTEGRAL_TYPE_P (type)))
        return fold_build2_loc (loc, PLUS_EXPR, type,
                            fold_convert_loc (loc, type, arg0),
                            fold_convert_loc (loc, type,
                                              negate_expr (arg1)));

ah, no.  The above only applies to float-type and integral-types.

Thus yes, your patch is ok.  Can you double-check the other pattern,

/* -(A + B) -> (-B) - A.  */
(simplify
 (negate (plus:c @0 negate_expr_p@1))
 (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
      && !HONOR_SIGNED_ZEROS (element_mode (type)))
  (minus (negate @1) @0)))

?

Thanks,
Richard.

> Thanks,
> Kyrill
> 
> 
> 2015-07-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR middle-end/66915
>     * match.pd (A - B -> A + (-B)): Don't allow folding
>     when type if a fixed-point type.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Kyrill Tkachov]

On 21/07/15 08:24, Richard Biener wrote:
> On Mon, 20 Jul 2015, Kyrill Tkachov wrote:
>
>> Hi all,
>>
>> This patch fixes the PR in question which is a miscompilation of
>> gcc.dg/fixed-point/unary.c on arm.
>> It just restricts the A - B -> A + (-B) transformation when the type is
>> fixed-point.
>>
>> This fixes the testcase for me.
>> Is this the right approach?
>>
>> Bootstrap and test on arm and x86 running.
>>
>> Ok if testing is clean?
> Ok, but I think the fold-const.c code has the same issue, no:
>
>        /* A - B -> A + (-B) if B is easily negatable.  */
>        if (negate_expr_p (arg1)
>            && !TYPE_OVERFLOW_SANITIZED (type)
>            && ((FLOAT_TYPE_P (type)
>                 /* Avoid this transformation if B is a positive REAL_CST.
> */
>                 && (TREE_CODE (arg1) != REAL_CST
>                     ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
>                || INTEGRAL_TYPE_P (type)))
>          return fold_build2_loc (loc, PLUS_EXPR, type,
>                              fold_convert_loc (loc, type, arg0),
>                              fold_convert_loc (loc, type,
>                                                negate_expr (arg1)));
>
> ah, no.  The above only applies to float-type and integral-types.
>
> Thus yes, your patch is ok.  Can you double-check the other pattern,
>
> /* -(A + B) -> (-B) - A.  */
> (simplify
>   (negate (plus:c @0 negate_expr_p@1))
>   (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
>        && !HONOR_SIGNED_ZEROS (element_mode (type)))
>    (minus (negate @1) @0)))
>
> ?

Thanks, committed with r226028.
I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition.
That would more closely mirror the original logic, right?
That passes x86_64 bootstrap and aarch64 testing looks ok.

>
> Thanks,
> Richard.
>
>> Thanks,
>> Kyrill
>>
>>
>> 2015-07-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR middle-end/66915
>>      * match.pd (A - B -> A + (-B)): Don't allow folding
>>      when type if a fixed-point type.
>>

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Richard Biener]

On Tue, 21 Jul 2015, Kyrill Tkachov wrote:

> 
> On 21/07/15 08:24, Richard Biener wrote:
> > On Mon, 20 Jul 2015, Kyrill Tkachov wrote:
> > 
> > > Hi all,
> > > 
> > > This patch fixes the PR in question which is a miscompilation of
> > > gcc.dg/fixed-point/unary.c on arm.
> > > It just restricts the A - B -> A + (-B) transformation when the type is
> > > fixed-point.
> > > 
> > > This fixes the testcase for me.
> > > Is this the right approach?
> > > 
> > > Bootstrap and test on arm and x86 running.
> > > 
> > > Ok if testing is clean?
> > Ok, but I think the fold-const.c code has the same issue, no:
> > 
> >        /* A - B -> A + (-B) if B is easily negatable.  */
> >        if (negate_expr_p (arg1)
> >            && !TYPE_OVERFLOW_SANITIZED (type)
> >            && ((FLOAT_TYPE_P (type)
> >                 /* Avoid this transformation if B is a positive REAL_CST.
> > */
> >                 && (TREE_CODE (arg1) != REAL_CST
> >                     ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
> >                || INTEGRAL_TYPE_P (type)))
> >          return fold_build2_loc (loc, PLUS_EXPR, type,
> >                              fold_convert_loc (loc, type, arg0),
> >                              fold_convert_loc (loc, type,
> >                                                negate_expr (arg1)));
> > 
> > ah, no.  The above only applies to float-type and integral-types.
> > 
> > Thus yes, your patch is ok.  Can you double-check the other pattern,
> > 
> > /* -(A + B) -> (-B) - A.  */
> > (simplify
> >   (negate (plus:c @0 negate_expr_p@1))
> >   (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> >        && !HONOR_SIGNED_ZEROS (element_mode (type)))
> >    (minus (negate @1) @0)))
> > 
> > ?
> 
> Thanks, committed with r226028.
> I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition.
> That would more closely mirror the original logic, right?
> That passes x86_64 bootstrap and aarch64 testing looks ok.

Yeah, that works for me, too.

Thanks,
Richard.

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]


On 21/07/15 11:11, Richard Biener wrote:
> On Tue, 21 Jul 2015, Kyrill Tkachov wrote:
>
>> On 21/07/15 08:24, Richard Biener wrote:
>>> On Mon, 20 Jul 2015, Kyrill Tkachov wrote:
>>>
>>>> Hi all,
>>>>
>>>> This patch fixes the PR in question which is a miscompilation of
>>>> gcc.dg/fixed-point/unary.c on arm.
>>>> It just restricts the A - B -> A + (-B) transformation when the type is
>>>> fixed-point.
>>>>
>>>> This fixes the testcase for me.
>>>> Is this the right approach?
>>>>
>>>> Bootstrap and test on arm and x86 running.
>>>>
>>>> Ok if testing is clean?
>>> Ok, but I think the fold-const.c code has the same issue, no:
>>>
>>>         /* A - B -> A + (-B) if B is easily negatable.  */
>>>         if (negate_expr_p (arg1)
>>>             && !TYPE_OVERFLOW_SANITIZED (type)
>>>             && ((FLOAT_TYPE_P (type)
>>>                  /* Avoid this transformation if B is a positive REAL_CST.
>>> */
>>>                  && (TREE_CODE (arg1) != REAL_CST
>>>                      ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
>>>                 || INTEGRAL_TYPE_P (type)))
>>>           return fold_build2_loc (loc, PLUS_EXPR, type,
>>>                               fold_convert_loc (loc, type, arg0),
>>>                               fold_convert_loc (loc, type,
>>>                                                 negate_expr (arg1)));
>>>
>>> ah, no.  The above only applies to float-type and integral-types.
>>>
>>> Thus yes, your patch is ok.  Can you double-check the other pattern,
>>>
>>> /* -(A + B) -> (-B) - A.  */
>>> (simplify
>>>    (negate (plus:c @0 negate_expr_p@1))
>>>    (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
>>>         && !HONOR_SIGNED_ZEROS (element_mode (type)))
>>>     (minus (negate @1) @0)))
>>>
>>> ?
>> Thanks, committed with r226028.
>> I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the condition.
>> That would more closely mirror the original logic, right?
>> That passes x86_64 bootstrap and aarch64 testing looks ok.
> Yeah, that works for me, too.

How about this patch then?
Bootstrapped and tested on x86_64 and aarch64.

Thanks,
Kyrill

2015-07-23  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * match.pd (-(A + B) -> (-B) - A): Restrict to floating point
     and integral types.


>
> Thanks,
> Richard.
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: matchpd-integral-float.patch --]
[-- Type: text/x-patch; name=matchpd-integral-float.patch, Size: 750 bytes --]

commit d514c81a7965fd24b9d8c294b12179b2369c8aa4
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Jul 21 10:18:31 2015 +0100

    [match.pd] Restrict -(A + B) -> (-B) - A to integral or float types

diff --git a/gcc/match.pd b/gcc/match.pd
index 3d7b32e..29367f2 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -515,7 +515,8 @@ along with GCC; see the file COPYING3.  If not see
 /* -(A + B) -> (-B) - A.  */
 (simplify
  (negate (plus:c @0 negate_expr_p@1))
- (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
+ (if ((FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type))
+      && !HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
       && !HONOR_SIGNED_ZEROS (element_mode (type)))
   (minus (negate @1) @0)))
 

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Richard Biener]

On Thu, 23 Jul 2015, Kyrill Tkachov wrote:

> 
> On 21/07/15 11:11, Richard Biener wrote:
> > On Tue, 21 Jul 2015, Kyrill Tkachov wrote:
> > 
> > > On 21/07/15 08:24, Richard Biener wrote:
> > > > On Mon, 20 Jul 2015, Kyrill Tkachov wrote:
> > > > 
> > > > > Hi all,
> > > > > 
> > > > > This patch fixes the PR in question which is a miscompilation of
> > > > > gcc.dg/fixed-point/unary.c on arm.
> > > > > It just restricts the A - B -> A + (-B) transformation when the type
> > > > > is
> > > > > fixed-point.
> > > > > 
> > > > > This fixes the testcase for me.
> > > > > Is this the right approach?
> > > > > 
> > > > > Bootstrap and test on arm and x86 running.
> > > > > 
> > > > > Ok if testing is clean?
> > > > Ok, but I think the fold-const.c code has the same issue, no:
> > > > 
> > > >         /* A - B -> A + (-B) if B is easily negatable.  */
> > > >         if (negate_expr_p (arg1)
> > > >             && !TYPE_OVERFLOW_SANITIZED (type)
> > > >             && ((FLOAT_TYPE_P (type)
> > > >                  /* Avoid this transformation if B is a positive
> > > > REAL_CST.
> > > > */
> > > >                  && (TREE_CODE (arg1) != REAL_CST
> > > >                      ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
> > > >                 || INTEGRAL_TYPE_P (type)))
> > > >           return fold_build2_loc (loc, PLUS_EXPR, type,
> > > >                               fold_convert_loc (loc, type, arg0),
> > > >                               fold_convert_loc (loc, type,
> > > >                                                 negate_expr (arg1)));
> > > > 
> > > > ah, no.  The above only applies to float-type and integral-types.
> > > > 
> > > > Thus yes, your patch is ok.  Can you double-check the other pattern,
> > > > 
> > > > /* -(A + B) -> (-B) - A.  */
> > > > (simplify
> > > >    (negate (plus:c @0 negate_expr_p@1))
> > > >    (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
> > > >         && !HONOR_SIGNED_ZEROS (element_mode (type)))
> > > >     (minus (negate @1) @0)))
> > > > 
> > > > ?
> > > Thanks, committed with r226028.
> > > I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the
> > > condition.
> > > That would more closely mirror the original logic, right?
> > > That passes x86_64 bootstrap and aarch64 testing looks ok.
> > Yeah, that works for me, too.
> 
> How about this patch then?
> Bootstrapped and tested on x86_64 and aarch64.

Hmm.  The code already pretty much matches the one in fold-const.c.

So what's the actual issue with fixed-point types and
-(A + B) -> -B - A iff negate_expr_p says that B can be
safely negated?

That is, can you add a testcase that fails without the patch?

Thanks
Richard.

Re: [PATCH][match.pd] PR middle-end/66915 Restrict A - B -> A + (-B) to non-fixed-point types

[Kyrill Tkachov]

On 23/07/15 12:16, Richard Biener wrote:
> On Thu, 23 Jul 2015, Kyrill Tkachov wrote:
>
>> On 21/07/15 11:11, Richard Biener wrote:
>>> On Tue, 21 Jul 2015, Kyrill Tkachov wrote:
>>>
>>>> On 21/07/15 08:24, Richard Biener wrote:
>>>>> On Mon, 20 Jul 2015, Kyrill Tkachov wrote:
>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> This patch fixes the PR in question which is a miscompilation of
>>>>>> gcc.dg/fixed-point/unary.c on arm.
>>>>>> It just restricts the A - B -> A + (-B) transformation when the type
>>>>>> is
>>>>>> fixed-point.
>>>>>>
>>>>>> This fixes the testcase for me.
>>>>>> Is this the right approach?
>>>>>>
>>>>>> Bootstrap and test on arm and x86 running.
>>>>>>
>>>>>> Ok if testing is clean?
>>>>> Ok, but I think the fold-const.c code has the same issue, no:
>>>>>
>>>>>          /* A - B -> A + (-B) if B is easily negatable.  */
>>>>>          if (negate_expr_p (arg1)
>>>>>              && !TYPE_OVERFLOW_SANITIZED (type)
>>>>>              && ((FLOAT_TYPE_P (type)
>>>>>                   /* Avoid this transformation if B is a positive
>>>>> REAL_CST.
>>>>> */
>>>>>                   && (TREE_CODE (arg1) != REAL_CST
>>>>>                       ||  REAL_VALUE_NEGATIVE (TREE_REAL_CST (arg1))))
>>>>>                  || INTEGRAL_TYPE_P (type)))
>>>>>            return fold_build2_loc (loc, PLUS_EXPR, type,
>>>>>                                fold_convert_loc (loc, type, arg0),
>>>>>                                fold_convert_loc (loc, type,
>>>>>                                                  negate_expr (arg1)));
>>>>>
>>>>> ah, no.  The above only applies to float-type and integral-types.
>>>>>
>>>>> Thus yes, your patch is ok.  Can you double-check the other pattern,
>>>>>
>>>>> /* -(A + B) -> (-B) - A.  */
>>>>> (simplify
>>>>>     (negate (plus:c @0 negate_expr_p@1))
>>>>>     (if (!HONOR_SIGN_DEPENDENT_ROUNDING (element_mode (type))
>>>>>          && !HONOR_SIGNED_ZEROS (element_mode (type)))
>>>>>      (minus (negate @1) @0)))
>>>>>
>>>>> ?
>>>> Thanks, committed with r226028.
>>>> I can add (FLOAT_TYPE_P (type) || INTEGRAL_TYPE_P (type)) to the
>>>> condition.
>>>> That would more closely mirror the original logic, right?
>>>> That passes x86_64 bootstrap and aarch64 testing looks ok.
>>> Yeah, that works for me, too.
>> How about this patch then?
>> Bootstrapped and tested on x86_64 and aarch64.
> Hmm.  The code already pretty much matches the one in fold-const.c.
>
> So what's the actual issue with fixed-point types and
> -(A + B) -> -B - A iff negate_expr_p says that B can be
> safely negated?
>
> That is, can you add a testcase that fails without the patch?

I don't have such a testcase.
If negate_expr_p does what we want here, then I suppose it's redundant
and I withdraw the patch.
I'm not very familiar with the fold-const.c code...

Kyrill

>
> Thanks
> Richard.
>