diff mbox series

[2/3] Change the `zero_one ==/!= 0) ? y : z <op> y` patterns to use multiply rather than `(-zero_one) & z`

Message ID 20230607213217.3052696-2-apinski@marvell.com
State Unresolved
Headers
Series [1/3] MATCH: Allow unsigned types for `X & -Y -> X * Y` pattern |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Andrew Pinski June 7, 2023, 9:32 p.m. UTC 
  Since there is a pattern to convert `(-zero_one) & z` into `zero_one * z` already,
it is better if we don't do a secondary transformation. This reduces the extra
statements produced by match-and-simplify on the gimple level too.

gcc/ChangeLog:

	* match.pd (`zero_one ==/!= 0) ? y : z <op> y`): Use
	multiply rather than negation/bit_and.
---
 gcc/match.pd | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Jeff Law June 7, 2023, 10:56 p.m. UTC | #1 
On 6/7/23 15:32, Andrew Pinski via Gcc-patches wrote:
> Since there is a pattern to convert `(-zero_one) & z` into `zero_one * z` already,
> it is better if we don't do a secondary transformation. This reduces the extra
> statements produced by match-and-simplify on the gimple level too.
> 
> gcc/ChangeLog:
> 
> 	* match.pd (`zero_one ==/!= 0) ? y : z <op> y`): Use
> 	multiply rather than negation/bit_and.
Don't you need to check the types in a manner similar to what the A & -Y 
-> X * Y pattern does before you make this transformation?

jeff
Andrew Pinski June 7, 2023, 11:05 p.m. UTC | #2 
On Wed, Jun 7, 2023 at 3:57 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 6/7/23 15:32, Andrew Pinski via Gcc-patches wrote:
> > Since there is a pattern to convert `(-zero_one) & z` into `zero_one * z` already,
> > it is better if we don't do a secondary transformation. This reduces the extra
> > statements produced by match-and-simplify on the gimple level too.
> >
> > gcc/ChangeLog:
> >
> >       * match.pd (`zero_one ==/!= 0) ? y : z <op> y`): Use
> >       multiply rather than negation/bit_and.
> Don't you need to check the types in a manner similar to what the A & -Y
> -> X * Y pattern does before you make this transformation?

No, because the convert is in a different order than in that
transformation; a very subtle difference which makes it work.

In A & -Y it was matching:
(bit_and  (convert? (negate
But here we have:
(bit_and (negate (convert
Notice the convert is in a different location, in the `A & -Y` case,
the convert needs to be a sign extending (or a truncation) of the
negative value. Here we are converting the one_zero_value to the new
type so we get zero_one in the new type and then doing the negation
getting us 0 or -1 value.

Thanks,
Andrew

>
> jeff
>
Jeff Law June 7, 2023, 11:11 p.m. UTC | #3 
On 6/7/23 17:05, Andrew Pinski wrote:
> On Wed, Jun 7, 2023 at 3:57 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 6/7/23 15:32, Andrew Pinski via Gcc-patches wrote:
>>> Since there is a pattern to convert `(-zero_one) & z` into `zero_one * z` already,
>>> it is better if we don't do a secondary transformation. This reduces the extra
>>> statements produced by match-and-simplify on the gimple level too.
>>>
>>> gcc/ChangeLog:
>>>
>>>        * match.pd (`zero_one ==/!= 0) ? y : z <op> y`): Use
>>>        multiply rather than negation/bit_and.
>> Don't you need to check the types in a manner similar to what the A & -Y
>> -> X * Y pattern does before you make this transformation?
> 
> No, because the convert is in a different order than in that
> transformation; a very subtle difference which makes it work.
> 
> In A & -Y it was matching:
> (bit_and  (convert? (negate
> But here we have:
> (bit_and (negate (convert
> Notice the convert is in a different location, in the `A & -Y` case,
> the convert needs to be a sign extending (or a truncation) of the
> negative value. Here we are converting the one_zero_value to the new
> type so we get zero_one in the new type and then doing the negation
> getting us 0 or -1 value.
THanks for the clarification.  OK for the trunk.

jeff
Andrew Pinski June 7, 2023, 11:19 p.m. UTC | #4 
On Wed, Jun 7, 2023 at 4:11 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 6/7/23 17:05, Andrew Pinski wrote:
> > On Wed, Jun 7, 2023 at 3:57 PM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >>
> >> On 6/7/23 15:32, Andrew Pinski via Gcc-patches wrote:
> >>> Since there is a pattern to convert `(-zero_one) & z` into `zero_one * z` already,
> >>> it is better if we don't do a secondary transformation. This reduces the extra
> >>> statements produced by match-and-simplify on the gimple level too.
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>        * match.pd (`zero_one ==/!= 0) ? y : z <op> y`): Use
> >>>        multiply rather than negation/bit_and.
> >> Don't you need to check the types in a manner similar to what the A & -Y
> >> -> X * Y pattern does before you make this transformation?
> >
> > No, because the convert is in a different order than in that
> > transformation; a very subtle difference which makes it work.
> >
> > In A & -Y it was matching:
> > (bit_and  (convert? (negate
> > But here we have:
> > (bit_and (negate (convert
> > Notice the convert is in a different location, in the `A & -Y` case,
> > the convert needs to be a sign extending (or a truncation) of the
> > negative value. Here we are converting the one_zero_value to the new
> > type so we get zero_one in the new type and then doing the negation
> > getting us 0 or -1 value.
> THanks for the clarification.  OK for the trunk.

So even though my transformation is correct based on what was done in
match.pd but that was broken already for signed one bit integers:
```
struct s
{
  int t : 1;
};
int f(struct s t, int a, int b)
{
        int bd = t.t;
        if (bd) a|=b;
        return a;
}
```
I am going to withdraw this patch and fix that up first.

Thanks,
Andrew

>
> jeff
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 7b95b63cee4..c38b39fb45c 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3688,7 +3688,7 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (cond (le @0 integer_zerop@1) (negate@2 @0) integer_zerop@1)
   (max @2 @1))
 
-/* (zero_one == 0) ? y : z <op> y -> (-(typeof(y))zero_one & z) <op> y */
+/* (zero_one == 0) ? y : z <op> y -> ((typeof(y))zero_one * z) <op> y */
 (for op (bit_xor bit_ior)
  (simplify
   (cond (eq zero_one_valued_p@0
@@ -3698,9 +3698,9 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (if (INTEGRAL_TYPE_P (type)
        && TYPE_PRECISION (type) > 1
        && (INTEGRAL_TYPE_P (TREE_TYPE (@0))))
-       (op (bit_and (negate (convert:type @0)) @2) @1))))
+       (op (mult (convert:type @0) @2) @1))))
 
-/* (zero_one != 0) ? z <op> y : y -> (-(typeof(y))zero_one & z) <op> y */
+/* (zero_one != 0) ? z <op> y : y -> ((typeof(y))zero_one * z) <op> y */
 (for op (bit_xor bit_ior)
  (simplify
   (cond (ne zero_one_valued_p@0
@@ -3710,7 +3710,7 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (if (INTEGRAL_TYPE_P (type)
        && TYPE_PRECISION (type) > 1
        && (INTEGRAL_TYPE_P (TREE_TYPE (@0))))
-       (op (bit_and (negate (convert:type @0)) @2) @1))))
+       (op (mult (convert:type @0) @2) @1))))
 
 /* Simplifications of shift and rotates.  */