[PATCH] Move ~X | X -> -1 folding

[PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

This moves a simple optimization.  Here it's plain to see how :c
removes the need to duplicate code to handle commutativity.

I put some more converts into the pattern, but then it's turned
out that I also need the tree_nop_conversion_p (otherwise we'd
regress binop-notor2.c that uses booleans).

I did a regtest with the patterns in fold-const.c removed to see
whether we have some testing for this folding -- and there were
no regressions, so I had to write a test.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-06-29  Marek Polacek  <polacek@redhat.com>

	* fold-const.c (fold_binary_loc): Move ~X | X folding ...
	* match.pd: ... here.

	* gcc.dg/fold-ior-2.c: New test.

diff --git gcc/fold-const.c gcc/fold-const.c
index a447452..caba0cf 100644
--- gcc/fold-const.c
+++ gcc/fold-const.c
@@ -10928,24 +10928,6 @@ fold_binary_loc (location_t loc,
 
     case BIT_IOR_EXPR:
     bit_ior:
-      /* ~X | X is -1.  */
-      if (TREE_CODE (arg0) == BIT_NOT_EXPR
-	  && operand_equal_p (TREE_OPERAND (arg0, 0), arg1, 0))
-	{
-	  t1 = build_zero_cst (type);
-	  t1 = fold_unary_loc (loc, BIT_NOT_EXPR, type, t1);
-	  return omit_one_operand_loc (loc, type, t1, arg1);
-	}
-
-      /* X | ~X is -1.  */
-      if (TREE_CODE (arg1) == BIT_NOT_EXPR
-	  && operand_equal_p (arg0, TREE_OPERAND (arg1, 0), 0))
-	{
-	  t1 = build_zero_cst (type);
-	  t1 = fold_unary_loc (loc, BIT_NOT_EXPR, type, t1);
-	  return omit_one_operand_loc (loc, type, t1, arg0);
-	}
-
       /* Canonicalize (X & C1) | C2.  */
       if (TREE_CODE (arg0) == BIT_AND_EXPR
 	  && TREE_CODE (arg1) == INTEGER_CST
diff --git gcc/match.pd gcc/match.pd
index 0cf3d21..5dcbc1a 100644
--- gcc/match.pd
+++ gcc/match.pd
@@ -283,6 +283,12 @@ along with GCC; see the file COPYING3.  If not see
   (bit_and @0 integer_zerop@1)
   @1)
 
+/* ~x | x -> -1 */
+(simplify
+ (bit_ior:c (convert? @0) (convert? (bit_not @0)))
+ (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
+  { build_all_ones_cst (type); }))
+
 /* x ^ x -> 0 */
 (simplify
   (bit_xor @0 @0)
diff --git gcc/testsuite/gcc.dg/fold-ior-2.c gcc/testsuite/gcc.dg/fold-ior-2.c
index e69de29..6abac9e 100644
--- gcc/testsuite/gcc.dg/fold-ior-2.c
+++ gcc/testsuite/gcc.dg/fold-ior-2.c
@@ -0,0 +1,47 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-cddce1" } */
+
+int
+fn1 (int x)
+{
+  return ~x | x;
+}
+
+int
+fn2 (int x)
+{
+  return x | ~x;
+}
+
+unsigned int
+fn3 (unsigned int x)
+{
+  return ~x | x;
+}
+
+unsigned int
+fn4 (unsigned int x)
+{
+  return ~x | x;
+}
+
+int
+fn5 (int x)
+{
+  return ~x | (unsigned) x;
+}
+
+int
+fn6 (int x)
+{
+  return (unsigned) ~x | x;
+}
+
+int
+fn7 (int x)
+{
+  return ~(unsigned) x | x;
+}
+
+/* { dg-final { scan-tree-dump-not "~" "cddce1" } } */
+/* { dg-final { scan-tree-dump-not " \\| " "cddce1" } } */

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> This moves a simple optimization.  Here it's plain to see how :c
> removes the need to duplicate code to handle commutativity.
> 
> I put some more converts into the pattern, but then it's turned
> out that I also need the tree_nop_conversion_p (otherwise we'd
> regress binop-notor2.c that uses booleans).
> 
> I did a regtest with the patterns in fold-const.c removed to see
> whether we have some testing for this folding -- and there were
> no regressions, so I had to write a test.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2015-06-29  Marek Polacek  <polacek@redhat.com>
> 
> 	* fold-const.c (fold_binary_loc): Move ~X | X folding ...
> 	* match.pd: ... here.
> 
> 	* gcc.dg/fold-ior-2.c: New test.
> 
> diff --git gcc/fold-const.c gcc/fold-const.c
> index a447452..caba0cf 100644
> --- gcc/fold-const.c
> +++ gcc/fold-const.c
> @@ -10928,24 +10928,6 @@ fold_binary_loc (location_t loc,
>  
>      case BIT_IOR_EXPR:
>      bit_ior:
> -      /* ~X | X is -1.  */
> -      if (TREE_CODE (arg0) == BIT_NOT_EXPR
> -	  && operand_equal_p (TREE_OPERAND (arg0, 0), arg1, 0))
> -	{
> -	  t1 = build_zero_cst (type);
> -	  t1 = fold_unary_loc (loc, BIT_NOT_EXPR, type, t1);
> -	  return omit_one_operand_loc (loc, type, t1, arg1);
> -	}
> -
> -      /* X | ~X is -1.  */
> -      if (TREE_CODE (arg1) == BIT_NOT_EXPR
> -	  && operand_equal_p (arg0, TREE_OPERAND (arg1, 0), 0))
> -	{
> -	  t1 = build_zero_cst (type);
> -	  t1 = fold_unary_loc (loc, BIT_NOT_EXPR, type, t1);
> -	  return omit_one_operand_loc (loc, type, t1, arg0);
> -	}
> -
>        /* Canonicalize (X & C1) | C2.  */
>        if (TREE_CODE (arg0) == BIT_AND_EXPR
>  	  && TREE_CODE (arg1) == INTEGER_CST
> diff --git gcc/match.pd gcc/match.pd
> index 0cf3d21..5dcbc1a 100644
> --- gcc/match.pd
> +++ gcc/match.pd
> @@ -283,6 +283,12 @@ along with GCC; see the file COPYING3.  If not see
>    (bit_and @0 integer_zerop@1)
>    @1)
>  
> +/* ~x | x -> -1 */
> +(simplify
> + (bit_ior:c (convert? @0) (convert? (bit_not @0)))
> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
> +  { build_all_ones_cst (type); }))
> +
>  /* x ^ x -> 0 */
>  (simplify
>    (bit_xor @0 @0)
> diff --git gcc/testsuite/gcc.dg/fold-ior-2.c gcc/testsuite/gcc.dg/fold-ior-2.c
> index e69de29..6abac9e 100644
> --- gcc/testsuite/gcc.dg/fold-ior-2.c
> +++ gcc/testsuite/gcc.dg/fold-ior-2.c
> @@ -0,0 +1,47 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fdump-tree-cddce1" } */
> +
> +int
> +fn1 (int x)
> +{
> +  return ~x | x;
> +}
> +
> +int
> +fn2 (int x)
> +{
> +  return x | ~x;
> +}
> +
> +unsigned int
> +fn3 (unsigned int x)
> +{
> +  return ~x | x;
> +}
> +
> +unsigned int
> +fn4 (unsigned int x)
> +{
> +  return ~x | x;
> +}
> +
> +int
> +fn5 (int x)
> +{
> +  return ~x | (unsigned) x;
> +}
> +
> +int
> +fn6 (int x)
> +{
> +  return (unsigned) ~x | x;
> +}
> +
> +int
> +fn7 (int x)
> +{
> +  return ~(unsigned) x | x;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "~" "cddce1" } } */
> +/* { dg-final { scan-tree-dump-not " \\| " "cddce1" } } */
> 
> 	Marek
> 
> 

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

Re: [PATCH] Move ~X | X -> -1 folding

[Marc Glisse]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> This moves a simple optimization.  Here it's plain to see how :c
> removes the need to duplicate code to handle commutativity.

Note that the same transformation would work for plus and xor.

> I put some more converts into the pattern, but then it's turned
> out that I also need the tree_nop_conversion_p (otherwise we'd
> regress binop-notor2.c that uses booleans).

I don't really see why removing tree_nop_conversion_p would regress 
anything (though you would probably need to build the all_ones constant in 
TREE_TYPE (@0) and convert that to type). For my curiosity, could you 
explain a bit more?

-- 
Marc Glisse

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marc Glisse wrote:

> On Tue, 30 Jun 2015, Marek Polacek wrote:
> 
> > This moves a simple optimization.  Here it's plain to see how :c
> > removes the need to duplicate code to handle commutativity.
> 
> Note that the same transformation would work for plus and xor.
>
> > I put some more converts into the pattern, but then it's turned
> > out that I also need the tree_nop_conversion_p (otherwise we'd
> > regress binop-notor2.c that uses booleans).
> 
> I don't really see why removing tree_nop_conversion_p would regress anything
> (though you would probably need to build the all_ones constant in TREE_TYPE
> (@0) and convert that to type). For my curiosity, could you explain a bit
> more?

what fold-const.c did was certainly ensuring tree_nop_conversion_p but
indeed I also can't see why it should be necessary.  Marek said
for bools, but they are just signed one-bit precision entities...

Richard.

Re: [PATCH] Move ~X | X -> -1 folding

[Bernhard Reutner-Fischer]

On June 30, 2015 10:46:21 AM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote:

>--- gcc/testsuite/gcc.dg/fold-ior-2.c
>+++ gcc/testsuite/gcc.dg/fold-ior-2.c
>@@ -0,0 +1,47 @@
>+/* { dg-do compile } */
>+/* { dg-options "-O -fdump-tree-cddce1" } */
>+
>+int
>+fn1 (int x)
>+{
>+  return ~x | x;
>+}
>+
>+int
>+fn2 (int x)
>+{
>+  return x | ~x;
>+}
>+
>+unsigned int
>+fn3 (unsigned int x)
>+{
>+  return ~x | x;
>+}
>+
>+unsigned int
>+fn4 (unsigned int x)
>+{
>+  return ~x | x;
>+}

What's the difference between fn3 and fn4?

Thanks,

>+
>+int
>+fn5 (int x)
>+{
>+  return ~x | (unsigned) x;
>+}
>+
>+int
>+fn6 (int x)
>+{
>+  return (unsigned) ~x | x;
>+}
>+
>+int
>+fn7 (int x)
>+{
>+  return ~(unsigned) x | x;
>+}
>+
>+/* { dg-final { scan-tree-dump-not "~" "cddce1" } } */
>+/* { dg-final { scan-tree-dump-not " \\| " "cddce1" } } */
>
>	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 11:08:35AM +0200, Marc Glisse wrote:
> On Tue, 30 Jun 2015, Marek Polacek wrote:
> 
> >This moves a simple optimization.  Here it's plain to see how :c
> >removes the need to duplicate code to handle commutativity.
> 
> Note that the same transformation would work for plus and xor.
 
Sounds like a good follow-up.  I think moving from fold-const.c to
match.pd ought to be 1:1 for clarity.  I'll prepare a patch to also
handle +/^.

> >I put some more converts into the pattern, but then it's turned
> >out that I also need the tree_nop_conversion_p (otherwise we'd
> >regress binop-notor2.c that uses booleans).
> 
> I don't really see why removing tree_nop_conversion_p would regress anything
> (though you would probably need to build the all_ones constant in TREE_TYPE
> (@0) and convert that to type). For my curiosity, could you explain a bit
> more?

This wasn't all that clear to me.  The testcase in question is 

int
foo (_Bool a, _Bool b)
{
  return (a | (a == 0)) | ((b ^ 1) | b); 
}

this ought to be simplified to "return 1".  Through various folding we
arrive at

(int) ~b | (int) b

so we'd turn that into -1 (all_ones_cst of type int).  But for boolean b
"~b | b" is always 1, right?

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> On Tue, Jun 30, 2015 at 11:08:35AM +0200, Marc Glisse wrote:
> > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > 
> > >This moves a simple optimization.  Here it's plain to see how :c
> > >removes the need to duplicate code to handle commutativity.
> > 
> > Note that the same transformation would work for plus and xor.
>  
> Sounds like a good follow-up.  I think moving from fold-const.c to
> match.pd ought to be 1:1 for clarity.

Yes, I agree fully here.

> I'll prepare a patch to also
> handle +/^.

Thanks.

> > >I put some more converts into the pattern, but then it's turned
> > >out that I also need the tree_nop_conversion_p (otherwise we'd
> > >regress binop-notor2.c that uses booleans).
> > 
> > I don't really see why removing tree_nop_conversion_p would regress anything
> > (though you would probably need to build the all_ones constant in TREE_TYPE
> > (@0) and convert that to type). For my curiosity, could you explain a bit
> > more?
> 
> This wasn't all that clear to me.  The testcase in question is 
> 
> int
> foo (_Bool a, _Bool b)
> {
>   return (a | (a == 0)) | ((b ^ 1) | b); 
> }
> 
> this ought to be simplified to "return 1".  Through various folding we
> arrive at
> 
> (int) ~b | (int) b
> 
> so we'd turn that into -1 (all_ones_cst of type int).  But for boolean b
> "~b | b" is always 1, right?

Actually our bools are signed ;)  Even for unsigned bools we'd
then simply build '1' via build_all_ones_cst.

Richard.

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 12:22:57PM +0200, Bernhard Reutner-Fischer wrote:
> >+unsigned int
> >+fn3 (unsigned int x)
> >+{
> >+  return ~x | x;
> >+}
> >+
> >+unsigned int
> >+fn4 (unsigned int x)
> >+{
> >+  return ~x | x;
> >+}
> 
> What's the difference between fn3 and fn4?

Oops, fixed.

2015-06-30  Marek Polacek  <polacek@redhat.com>

	* gcc.dg/fold-ior-2.c (fn4): Swap operands.

diff --git gcc/testsuite/gcc.dg/fold-ior-2.c gcc/testsuite/gcc.dg/fold-ior-2.c
index 6abac9e..41b372d 100644
--- gcc/testsuite/gcc.dg/fold-ior-2.c
+++ gcc/testsuite/gcc.dg/fold-ior-2.c
@@ -22,7 +22,7 @@ fn3 (unsigned int x)
 unsigned int
 fn4 (unsigned int x)
 {
-  return ~x | x;
+  return x | ~x;
 }
 
 int

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Marc Glisse]

On Tue, 30 Jun 2015, Marek Polacek wrote:

>>> I put some more converts into the pattern, but then it's turned
>>> out that I also need the tree_nop_conversion_p (otherwise we'd
>>> regress binop-notor2.c that uses booleans).
>>
>> I don't really see why removing tree_nop_conversion_p would regress anything
>> (though you would probably need to build the all_ones constant in TREE_TYPE
>> (@0) and convert that to type). For my curiosity, could you explain a bit
>> more?
>
> This wasn't all that clear to me.  The testcase in question is
>
> int
> foo (_Bool a, _Bool b)
> {
>  return (a | (a == 0)) | ((b ^ 1) | b);
> }
>
> this ought to be simplified to "return 1".  Through various folding we
> arrive at
>
> (int) ~b | (int) b
>
> so we'd turn that into -1 (all_ones_cst of type int).  But for boolean b
> "~b | b" is always 1, right?

Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and 
convert that to type" help for that?

-- 
Marc Glisse

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> convert that to type" help for that?

It appears to work, but it seems weird to me to create a integer constant
in one type and then immediately cast it to another type.

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > convert that to type" help for that?
> 
> It appears to work, but it seems weird to me to create a integer constant
> in one type and then immediately cast it to another type.

Yes.  Do you have a testcase now that fails using bools?

> 	Marek
> 
> 

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

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> On Tue, 30 Jun 2015, Marek Polacek wrote:
> 
> > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > convert that to type" help for that?
> > 
> > It appears to work, but it seems weird to me to create a integer constant
> > in one type and then immediately cast it to another type.
> 
> Yes.  Do you have a testcase now that fails using bools?

I don't have a testcase that fails with the pattern we currently have, i.e.
the one with tree_nop_conversion_p.

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > 
> > > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > > convert that to type" help for that?
> > > 
> > > It appears to work, but it seems weird to me to create a integer constant
> > > in one type and then immediately cast it to another type.
> > 
> > Yes.  Do you have a testcase now that fails using bools?
> 
> I don't have a testcase that fails with the pattern we currently have, i.e.
> the one with tree_nop_conversion_p.

I mean with removing tree_nop_conversion_p.

Richard.

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 03:13:14PM +0200, Richard Biener wrote:
> On Tue, 30 Jun 2015, Marek Polacek wrote:
> 
> > On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > 
> > > > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > > > convert that to type" help for that?
> > > > 
> > > > It appears to work, but it seems weird to me to create a integer constant
> > > > in one type and then immediately cast it to another type.
> > > 
> > > Yes.  Do you have a testcase now that fails using bools?
> > 
> > I don't have a testcase that fails with the pattern we currently have, i.e.
> > the one with tree_nop_conversion_p.
> 
> I mean with removing tree_nop_conversion_p.

Aha.  With tree_nop_conversion_p removed, gcc.dg/binop-notor2.c fails,
because there we optimize the return statement to "return -1" instead
of "return 1".
<https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02179.html>

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Marek Polacek]

On Tue, Jun 30, 2015 at 03:59:23PM +0200, Richard Biener wrote:
> On Tue, 30 Jun 2015, Marek Polacek wrote:
> 
> > On Tue, Jun 30, 2015 at 03:13:14PM +0200, Richard Biener wrote:
> > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > 
> > > > On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> > > > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > > > 
> > > > > > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > > > > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > > > > > convert that to type" help for that?
> > > > > > 
> > > > > > It appears to work, but it seems weird to me to create a integer constant
> > > > > > in one type and then immediately cast it to another type.
> > > > > 
> > > > > Yes.  Do you have a testcase now that fails using bools?
> > > > 
> > > > I don't have a testcase that fails with the pattern we currently have, i.e.
> > > > the one with tree_nop_conversion_p.
> > > 
> > > I mean with removing tree_nop_conversion_p.
> > 
> > Aha.  With tree_nop_conversion_p removed, gcc.dg/binop-notor2.c fails,
> > because there we optimize the return statement to "return -1" instead
> > of "return 1".
> > <https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02179.html>
> 
> Hmm ok.  That testcase is basically
> 
> int foo (_Bool a)
> {
>   return ((int) a) | ((int) ~a);
> }
> 
> where indeed with unsigned bool (yeah, our bool is unsigned) we
> get zero-extension on both arms.  Similar issue would show up with
> 
> int foo (unsigned char a)
> {
>   return ((int) a) | ((int) ~a);
> }
> 
> so it's not specific to bools.  So yes, the suggestion to
> do
> 
>   (convert { build_all_ones_cst (TREE_TYPE (@0)); })
> 
> would work here.

Ok, so do you want me to change that pattern to use this
(convert { build_all_ones_cst (TREE_TYPE (@0)); })
(along with a new test containing those two functions you mentioned)?

If so, is such a patch preapproved provided it passes the usual testing?

	Marek

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> On Tue, Jun 30, 2015 at 03:13:14PM +0200, Richard Biener wrote:
> > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > 
> > > On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> > > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > > 
> > > > > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > > > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > > > > convert that to type" help for that?
> > > > > 
> > > > > It appears to work, but it seems weird to me to create a integer constant
> > > > > in one type and then immediately cast it to another type.
> > > > 
> > > > Yes.  Do you have a testcase now that fails using bools?
> > > 
> > > I don't have a testcase that fails with the pattern we currently have, i.e.
> > > the one with tree_nop_conversion_p.
> > 
> > I mean with removing tree_nop_conversion_p.
> 
> Aha.  With tree_nop_conversion_p removed, gcc.dg/binop-notor2.c fails,
> because there we optimize the return statement to "return -1" instead
> of "return 1".
> <https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02179.html>

Hmm ok.  That testcase is basically

int foo (_Bool a)
{
  return ((int) a) | ((int) ~a);
}

where indeed with unsigned bool (yeah, our bool is unsigned) we
get zero-extension on both arms.  Similar issue would show up with

int foo (unsigned char a)
{
  return ((int) a) | ((int) ~a);
}

so it's not specific to bools.  So yes, the suggestion to
do

  (convert { build_all_ones_cst (TREE_TYPE (@0)); })

would work here.

Richard.

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

Re: [PATCH] Move ~X | X -> -1 folding

[Richard Biener]

On Tue, 30 Jun 2015, Marek Polacek wrote:

> On Tue, Jun 30, 2015 at 03:59:23PM +0200, Richard Biener wrote:
> > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > 
> > > On Tue, Jun 30, 2015 at 03:13:14PM +0200, Richard Biener wrote:
> > > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > > 
> > > > > On Tue, Jun 30, 2015 at 02:47:49PM +0200, Richard Biener wrote:
> > > > > > On Tue, 30 Jun 2015, Marek Polacek wrote:
> > > > > > 
> > > > > > > On Tue, Jun 30, 2015 at 01:39:29PM +0200, Marc Glisse wrote:
> > > > > > > > Does my suggestion to "build the all_ones constant in TREE_TYPE (@0) and
> > > > > > > > convert that to type" help for that?
> > > > > > > 
> > > > > > > It appears to work, but it seems weird to me to create a integer constant
> > > > > > > in one type and then immediately cast it to another type.
> > > > > > 
> > > > > > Yes.  Do you have a testcase now that fails using bools?
> > > > > 
> > > > > I don't have a testcase that fails with the pattern we currently have, i.e.
> > > > > the one with tree_nop_conversion_p.
> > > > 
> > > > I mean with removing tree_nop_conversion_p.
> > > 
> > > Aha.  With tree_nop_conversion_p removed, gcc.dg/binop-notor2.c fails,
> > > because there we optimize the return statement to "return -1" instead
> > > of "return 1".
> > > <https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02179.html>
> > 
> > Hmm ok.  That testcase is basically
> > 
> > int foo (_Bool a)
> > {
> >   return ((int) a) | ((int) ~a);
> > }
> > 
> > where indeed with unsigned bool (yeah, our bool is unsigned) we
> > get zero-extension on both arms.  Similar issue would show up with
> > 
> > int foo (unsigned char a)
> > {
> >   return ((int) a) | ((int) ~a);
> > }
> > 
> > so it's not specific to bools.  So yes, the suggestion to
> > do
> > 
> >   (convert { build_all_ones_cst (TREE_TYPE (@0)); })
> > 
> > would work here.
> 
> Ok, so do you want me to change that pattern to use this
> (convert { build_all_ones_cst (TREE_TYPE (@0)); })
> (along with a new test containing those two functions you mentioned)?
> 
> If so, is such a patch preapproved provided it passes the usual testing?

Yes.

Thanks,
Richard.