public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH]middle-end fix floating out of constants in conditionals
@ 2022-09-23  9:21 Tamar Christina
  2022-09-24 20:44 ` Jeff Law
  2022-09-26 10:28 ` Richard Biener
  0 siblings, 2 replies; 6+ messages in thread
From: Tamar Christina @ 2022-09-23  9:21 UTC (permalink / raw)
  To: gcc-patches; +Cc: nd, rguenther, jeffreyalaw

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

Hi All,

The following testcase:

int zoo1 (int a, int b, int c, int d)
{
   return (a > b ? c : d) & 1;
}

gets de-optimized by the front-end since somewhere around GCC 4.x due to a fix
that was added to fold_binary_op_with_conditional_arg.

The folding is supposed to succeed only if we have folded at least one of the
branches, however the check doesn't tests that all of the values are
non-constant.  So if one of the operators are a constant it accepts the folding.

This ends up folding

   return (a > b ? c : d) & 1;

into

   return (a > b ? c & 1 : d & 1);

and thus performing the AND twice.

change changes it to reject the folding if one of the arguments are a constant
and if the operations being performed are the same.

Secondly it adds a new match.pd rule to now also fold the opposite direction, so
it now also folds:

   return (a > b ? c & 1 : d & 1);

into

   return (a > b ? c : d) & 1;

Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
and <on-goin> issues.

Ok for master?

Thanks,
Tamar

gcc/ChangeLog:

	* fold-const.cc (fold_binary_op_with_conditional_arg): Add relaxation.
	* match.pd: Add ternary constant fold rule.
	* tree-cfg.cc (verify_gimple_assign_ternary): RHS1 of a COND_EXPR isn't
	a value but an expression itself. 

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/if-compare_3.c: New test.

--- inline copy of patch -- 
diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index 4f4ec81c8d4b6937ade3141a14c695b67c874c35..0ee083f290d12104969f1b335dc33917c97b4808 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -7212,7 +7212,9 @@ fold_binary_op_with_conditional_arg (location_t loc,
     }
 
   /* Check that we have simplified at least one of the branches.  */
-  if (!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
+  if ((!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
+      || (TREE_CONSTANT (arg) && TREE_CODE (lhs) == TREE_CODE (rhs)
+	  && !TREE_CONSTANT (lhs)))
     return NULL_TREE;
 
   return fold_build3_loc (loc, cond_code, type, test, lhs, rhs);
diff --git a/gcc/match.pd b/gcc/match.pd
index b225d36dc758f1581502c8d03761544bfd499c01..b61ed70e69b881a49177f10f20c1f92712bb8665 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4318,6 +4318,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (op @3 (vec_cond:s @0 @1 @2))
   (vec_cond @0 (op! @3 @1) (op! @3 @2))))
 
+/* Float out binary operations from branches if they can't be folded.
+   Fold (a ? (b op c) : (d op c)) --> (op (a ? b : d) c).  */
+(for op (plus mult min max bit_and bit_ior bit_xor minus lshift rshift rdiv
+	 trunc_div ceil_div floor_div round_div trunc_mod ceil_mod floor_mod
+	 round_mod)
+ (simplify
+  (cond @0 (op @1 @2) (op @3 @2))
+   (if (!FLOAT_TYPE_P (type) || !(HONOR_NANS (@1) && flag_trapping_math))
+    (op (cond @0 @1 @3) @2))))
+
 #if GIMPLE
 (match (nop_atomic_bit_test_and_p @0 @1 @4)
  (bit_and (convert?@4 (ATOMIC_FETCH_OR_XOR_N @2 INTEGER_CST@0 @3))
diff --git a/gcc/testsuite/gcc.target/aarch64/if-compare_3.c b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
new file mode 100644
index 0000000000000000000000000000000000000000..1d97da5c0d6454175881c219927471a567a6f0c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O3 -std=c99" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
+
+/*
+**zoo:
+**	cmp	w0, w1
+**	csel	w0, w3, w2, le
+**	ret
+*/
+int zoo (int a, int b, int c, int d)
+{
+   return a > b ? c : d;
+}
+
+/*
+**zoo1:
+**	cmp	w0, w1
+**	csel	w0, w3, w2, le
+**	and	w0, w0, 1
+**	ret
+*/
+int zoo1 (int a, int b, int c, int d)
+{
+   return (a > b ? c : d) & 1;
+}
+
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index b19710392940cf469de52d006603ae1e3deb6b76..aaf1b29da5c598add25dad2c38b828eaa89c49ce 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -4244,7 +4244,9 @@ verify_gimple_assign_ternary (gassign *stmt)
       return true;
     }
 
-  if (!is_gimple_val (rhs1)
+  /* In a COND_EXPR the rhs1 is the condition and thus isn't
+     a gimple value by definition.  */
+  if ((!is_gimple_val (rhs1) && rhs_code != COND_EXPR)
       || !is_gimple_val (rhs2)
       || !is_gimple_val (rhs3))
     {




-- 

[-- Attachment #2: rb15680.patch --]
[-- Type: text/plain, Size: 2881 bytes --]

diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index 4f4ec81c8d4b6937ade3141a14c695b67c874c35..0ee083f290d12104969f1b335dc33917c97b4808 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -7212,7 +7212,9 @@ fold_binary_op_with_conditional_arg (location_t loc,
     }
 
   /* Check that we have simplified at least one of the branches.  */
-  if (!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
+  if ((!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
+      || (TREE_CONSTANT (arg) && TREE_CODE (lhs) == TREE_CODE (rhs)
+	  && !TREE_CONSTANT (lhs)))
     return NULL_TREE;
 
   return fold_build3_loc (loc, cond_code, type, test, lhs, rhs);
diff --git a/gcc/match.pd b/gcc/match.pd
index b225d36dc758f1581502c8d03761544bfd499c01..b61ed70e69b881a49177f10f20c1f92712bb8665 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4318,6 +4318,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (op @3 (vec_cond:s @0 @1 @2))
   (vec_cond @0 (op! @3 @1) (op! @3 @2))))
 
+/* Float out binary operations from branches if they can't be folded.
+   Fold (a ? (b op c) : (d op c)) --> (op (a ? b : d) c).  */
+(for op (plus mult min max bit_and bit_ior bit_xor minus lshift rshift rdiv
+	 trunc_div ceil_div floor_div round_div trunc_mod ceil_mod floor_mod
+	 round_mod)
+ (simplify
+  (cond @0 (op @1 @2) (op @3 @2))
+   (if (!FLOAT_TYPE_P (type) || !(HONOR_NANS (@1) && flag_trapping_math))
+    (op (cond @0 @1 @3) @2))))
+
 #if GIMPLE
 (match (nop_atomic_bit_test_and_p @0 @1 @4)
  (bit_and (convert?@4 (ATOMIC_FETCH_OR_XOR_N @2 INTEGER_CST@0 @3))
diff --git a/gcc/testsuite/gcc.target/aarch64/if-compare_3.c b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
new file mode 100644
index 0000000000000000000000000000000000000000..1d97da5c0d6454175881c219927471a567a6f0c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O3 -std=c99" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
+
+/*
+**zoo:
+**	cmp	w0, w1
+**	csel	w0, w3, w2, le
+**	ret
+*/
+int zoo (int a, int b, int c, int d)
+{
+   return a > b ? c : d;
+}
+
+/*
+**zoo1:
+**	cmp	w0, w1
+**	csel	w0, w3, w2, le
+**	and	w0, w0, 1
+**	ret
+*/
+int zoo1 (int a, int b, int c, int d)
+{
+   return (a > b ? c : d) & 1;
+}
+
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index b19710392940cf469de52d006603ae1e3deb6b76..aaf1b29da5c598add25dad2c38b828eaa89c49ce 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -4244,7 +4244,9 @@ verify_gimple_assign_ternary (gassign *stmt)
       return true;
     }
 
-  if (!is_gimple_val (rhs1)
+  /* In a COND_EXPR the rhs1 is the condition and thus isn't
+     a gimple value by definition.  */
+  if ((!is_gimple_val (rhs1) && rhs_code != COND_EXPR)
       || !is_gimple_val (rhs2)
       || !is_gimple_val (rhs3))
     {




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]middle-end fix floating out of constants in conditionals
  2022-09-23  9:21 [PATCH]middle-end fix floating out of constants in conditionals Tamar Christina
@ 2022-09-24 20:44 ` Jeff Law
  2022-09-26 10:28 ` Richard Biener
  1 sibling, 0 replies; 6+ messages in thread
From: Jeff Law @ 2022-09-24 20:44 UTC (permalink / raw)
  To: Tamar Christina, gcc-patches; +Cc: nd, rguenther


On 9/23/22 03:21, Tamar Christina wrote:
> Hi All,
>
> The following testcase:
>
> int zoo1 (int a, int b, int c, int d)
> {
>     return (a > b ? c : d) & 1;
> }
>
> gets de-optimized by the front-end since somewhere around GCC 4.x due to a fix
> that was added to fold_binary_op_with_conditional_arg.
>
> The folding is supposed to succeed only if we have folded at least one of the
> branches, however the check doesn't tests that all of the values are
> non-constant.  So if one of the operators are a constant it accepts the folding.
>
> This ends up folding
>
>     return (a > b ? c : d) & 1;
>
> into
>
>     return (a > b ? c & 1 : d & 1);
>
> and thus performing the AND twice.
>
> change changes it to reject the folding if one of the arguments are a constant
> and if the operations being performed are the same.
>
> Secondly it adds a new match.pd rule to now also fold the opposite direction, so
> it now also folds:
>
>     return (a > b ? c & 1 : d & 1);
>
> into
>
>     return (a > b ? c : d) & 1;
>
> Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> and <on-goin> issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 	* fold-const.cc (fold_binary_op_with_conditional_arg): Add relaxation.
> 	* match.pd: Add ternary constant fold rule.
> 	* tree-cfg.cc (verify_gimple_assign_ternary): RHS1 of a COND_EXPR isn't
> 	a value but an expression itself.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/if-compare_3.c: New test.

OK

jeff



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]middle-end fix floating out of constants in conditionals
  2022-09-23  9:21 [PATCH]middle-end fix floating out of constants in conditionals Tamar Christina
  2022-09-24 20:44 ` Jeff Law
@ 2022-09-26 10:28 ` Richard Biener
  2022-09-26 11:08   ` Eric Botcazou
  2022-10-17  9:17   ` Tamar Christina
  1 sibling, 2 replies; 6+ messages in thread
From: Richard Biener @ 2022-09-26 10:28 UTC (permalink / raw)
  To: Tamar Christina; +Cc: gcc-patches, nd, jeffreyalaw, ebotcazou

On Fri, 23 Sep 2022, Tamar Christina wrote:

> Hi All,
> 
> The following testcase:
> 
> int zoo1 (int a, int b, int c, int d)
> {
>    return (a > b ? c : d) & 1;
> }
> 
> gets de-optimized by the front-end since somewhere around GCC 4.x due to a fix
> that was added to fold_binary_op_with_conditional_arg.
> 
> The folding is supposed to succeed only if we have folded at least one of the
> branches, however the check doesn't tests that all of the values are
> non-constant.  So if one of the operators are a constant it accepts the folding.
> 
> This ends up folding
> 
>    return (a > b ? c : d) & 1;
> 
> into
> 
>    return (a > b ? c & 1 : d & 1);
> 
> and thus performing the AND twice.
> 
> change changes it to reject the folding if one of the arguments are a constant
> and if the operations being performed are the same.
> 
> Secondly it adds a new match.pd rule to now also fold the opposite direction, so
> it now also folds:
> 
>    return (a > b ? c & 1 : d & 1);
> 
> into
> 
>    return (a > b ? c : d) & 1;
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> and <on-goin> issues.
> 
> Ok for master?
> 
> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	* fold-const.cc (fold_binary_op_with_conditional_arg): Add relaxation.
> 	* match.pd: Add ternary constant fold rule.
> 	* tree-cfg.cc (verify_gimple_assign_ternary): RHS1 of a COND_EXPR isn't
> 	a value but an expression itself. 
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/aarch64/if-compare_3.c: New test.
> 
> --- inline copy of patch -- 
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index 4f4ec81c8d4b6937ade3141a14c695b67c874c35..0ee083f290d12104969f1b335dc33917c97b4808 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -7212,7 +7212,9 @@ fold_binary_op_with_conditional_arg (location_t loc,
>      }
>  
>    /* Check that we have simplified at least one of the branches.  */
> -  if (!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
> +  if ((!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
> +      || (TREE_CONSTANT (arg) && TREE_CODE (lhs) == TREE_CODE (rhs)
> +	  && !TREE_CONSTANT (lhs)))
>      return NULL_TREE;

I think the better fix would be to only consider TREE_CONSTANT (arg)
if it wasn't constant initially.  Because clearly "simplify" intends
to be "constant" here.  In fact I wonder why we test !TREE_CONSTANT (arg)
at all, we don't simplify 'arg' ...

Eric added this test (previosuly we'd just always done the folding),
but I think not enough?

>  
>    return fold_build3_loc (loc, cond_code, type, test, lhs, rhs);
> diff --git a/gcc/match.pd b/gcc/match.pd
> index b225d36dc758f1581502c8d03761544bfd499c01..b61ed70e69b881a49177f10f20c1f92712bb8665 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -4318,6 +4318,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>    (op @3 (vec_cond:s @0 @1 @2))
>    (vec_cond @0 (op! @3 @1) (op! @3 @2))))
>  
> +/* Float out binary operations from branches if they can't be folded.
> +   Fold (a ? (b op c) : (d op c)) --> (op (a ? b : d) c).  */
> +(for op (plus mult min max bit_and bit_ior bit_xor minus lshift rshift rdiv
> +	 trunc_div ceil_div floor_div round_div trunc_mod ceil_mod floor_mod
> +	 round_mod)
> + (simplify
> +  (cond @0 (op @1 @2) (op @3 @2))
> +   (if (!FLOAT_TYPE_P (type) || !(HONOR_NANS (@1) && flag_trapping_math))
> +    (op (cond @0 @1 @3) @2))))

Ick.  Adding a reverse tranform is going to be prone to recursing :/

Why do you need to care about NANs or FP exceptions?  How do you know
if they can't be folded?  Since match.pd cannot handle arbitrary
operations it isn't a good fit for match.pd patterns, instead this would
be a forwprop pattern or, in case you want to catch GENERIC, a 
fold-const.cc one?

Thanks and sorry for the late reply - hope Jeffs approval didn't make
you apply it yet.

Richard.

> +
>  #if GIMPLE
>  (match (nop_atomic_bit_test_and_p @0 @1 @4)
>   (bit_and (convert?@4 (ATOMIC_FETCH_OR_XOR_N @2 INTEGER_CST@0 @3))
> diff --git a/gcc/testsuite/gcc.target/aarch64/if-compare_3.c b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1d97da5c0d6454175881c219927471a567a6f0c7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
> @@ -0,0 +1,27 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O3 -std=c99" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
> +
> +/*
> +**zoo:
> +**	cmp	w0, w1
> +**	csel	w0, w3, w2, le
> +**	ret
> +*/
> +int zoo (int a, int b, int c, int d)
> +{
> +   return a > b ? c : d;
> +}
> +
> +/*
> +**zoo1:
> +**	cmp	w0, w1
> +**	csel	w0, w3, w2, le
> +**	and	w0, w0, 1
> +**	ret
> +*/
> +int zoo1 (int a, int b, int c, int d)
> +{
> +   return (a > b ? c : d) & 1;
> +}
> +
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index b19710392940cf469de52d006603ae1e3deb6b76..aaf1b29da5c598add25dad2c38b828eaa89c49ce 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -4244,7 +4244,9 @@ verify_gimple_assign_ternary (gassign *stmt)
>        return true;
>      }
>  
> -  if (!is_gimple_val (rhs1)
> +  /* In a COND_EXPR the rhs1 is the condition and thus isn't
> +     a gimple value by definition.  */
> +  if ((!is_gimple_val (rhs1) && rhs_code != COND_EXPR)
>        || !is_gimple_val (rhs2)
>        || !is_gimple_val (rhs3))
>      {
> 
> 
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]middle-end fix floating out of constants in conditionals
  2022-09-26 10:28 ` Richard Biener
@ 2022-09-26 11:08   ` Eric Botcazou
  2022-09-26 11:12     ` Eric Botcazou
  2022-10-17  9:17   ` Tamar Christina
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2022-09-26 11:08 UTC (permalink / raw)
  To: Richard Biener; +Cc: Tamar Christina, gcc-patches, nd, gcc-patches

> I think the better fix would be to only consider TREE_CONSTANT (arg)
> if it wasn't constant initially.  Because clearly "simplify" intends
> to be "constant" here.  In fact I wonder why we test !TREE_CONSTANT (arg)
> at all, we don't simplify 'arg' ...
> 
> Eric added this test (previosuly we'd just always done the folding),
> but I think not enough?

Before my change we had always done the folding *only* for TREE_CONSTANT (arg)
and my change allowed it for some cases of !TREE_CONSTANT (arg), but I did not
want to touch the !TREE_CONSTANT (arg) case at all:

	* fold-const.c (fold_binary_op_with_conditional_arg): Do not restrict
	the folding to constants.  Remove redundant final conversion.

Tamar's issue appears to be for TREE_CONSTANT (arg) so orthogonal to mine.

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH]middle-end fix floating out of constants in conditionals
  2022-09-26 11:08   ` Eric Botcazou
@ 2022-09-26 11:12     ` Eric Botcazou
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Botcazou @ 2022-09-26 11:12 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, nd, gcc-patches

> Before my change we had always done the folding *only* for TREE_CONSTANT
> (arg) and my change allowed it for some cases of !TREE_CONSTANT (arg), but
> I did not want to touch the !TREE_CONSTANT (arg) case at all:

...to touch the TREE_CONSTANT (arg) case at all...

-- 
Eric Botcazou




^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH]middle-end fix floating out of constants in conditionals
  2022-09-26 10:28 ` Richard Biener
  2022-09-26 11:08   ` Eric Botcazou
@ 2022-10-17  9:17   ` Tamar Christina
  1 sibling, 0 replies; 6+ messages in thread
From: Tamar Christina @ 2022-10-17  9:17 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, nd, jeffreyalaw, ebotcazou

> -----Original Message-----
> From: Richard Biener <rguenther@suse.de>
> Sent: Monday, September 26, 2022 11:28 AM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; jeffreyalaw@gmail.com;
> ebotcazou@adacore.com
> Subject: Re: [PATCH]middle-end fix floating out of constants in conditionals
> 
> On Fri, 23 Sep 2022, Tamar Christina wrote:
> 
> > Hi All,
> >
> > The following testcase:
> >
> > int zoo1 (int a, int b, int c, int d)
> > {
> >    return (a > b ? c : d) & 1;
> > }
> >
> > gets de-optimized by the front-end since somewhere around GCC 4.x due
> > to a fix that was added to fold_binary_op_with_conditional_arg.
> >
> > The folding is supposed to succeed only if we have folded at least one
> > of the branches, however the check doesn't tests that all of the
> > values are non-constant.  So if one of the operators are a constant it
> accepts the folding.
> >
> > This ends up folding
> >
> >    return (a > b ? c : d) & 1;
> >
> > into
> >
> >    return (a > b ? c & 1 : d & 1);
> >
> > and thus performing the AND twice.
> >
> > change changes it to reject the folding if one of the arguments are a
> > constant and if the operations being performed are the same.
> >
> > Secondly it adds a new match.pd rule to now also fold the opposite
> > direction, so it now also folds:
> >
> >    return (a > b ? c & 1 : d & 1);
> >
> > into
> >
> >    return (a > b ? c : d) & 1;
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu
> > and <on-goin> issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 	* fold-const.cc (fold_binary_op_with_conditional_arg): Add
> relaxation.
> > 	* match.pd: Add ternary constant fold rule.
> > 	* tree-cfg.cc (verify_gimple_assign_ternary): RHS1 of a COND_EXPR
> isn't
> > 	a value but an expression itself.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 	* gcc.target/aarch64/if-compare_3.c: New test.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc index
> >
> 4f4ec81c8d4b6937ade3141a14c695b67c874c35..0ee083f290d12104969f1b335d
> c3
> > 3917c97b4808 100644
> > --- a/gcc/fold-const.cc
> > +++ b/gcc/fold-const.cc
> > @@ -7212,7 +7212,9 @@ fold_binary_op_with_conditional_arg (location_t
> loc,
> >      }
> >
> >    /* Check that we have simplified at least one of the branches.  */
> > -  if (!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) &&
> !TREE_CONSTANT
> > (rhs))
> > +  if ((!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) &&
> !TREE_CONSTANT (rhs))
> > +      || (TREE_CONSTANT (arg) && TREE_CODE (lhs) == TREE_CODE (rhs)
> > +	  && !TREE_CONSTANT (lhs)))
> >      return NULL_TREE;
> 
> I think the better fix would be to only consider TREE_CONSTANT (arg) if it
> wasn't constant initially.  Because clearly "simplify" intends to be "constant"
> here.  In fact I wonder why we test !TREE_CONSTANT (arg) at all, we don't
> simplify 'arg' ...

The function allows this because even when !TREE_CONSTANT (arg) the
true_value or false_value can instead be constant.

Yes it's of limited use unless true_value and false_value are 0 or 1.

> 
> Eric added this test (previosuly we'd just always done the folding), but I think
> not enough?
> 
> >
> >    return fold_build3_loc (loc, cond_code, type, test, lhs, rhs); diff
> > --git a/gcc/match.pd b/gcc/match.pd index
> >
> b225d36dc758f1581502c8d03761544bfd499c01..b61ed70e69b881a49177f10f20
> c1
> > f92712bb8665 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -4318,6 +4318,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >    (op @3 (vec_cond:s @0 @1 @2))
> >    (vec_cond @0 (op! @3 @1) (op! @3 @2))))
> >
> > +/* Float out binary operations from branches if they can't be folded.
> > +   Fold (a ? (b op c) : (d op c)) --> (op (a ? b : d) c).  */ (for op
> > +(plus mult min max bit_and bit_ior bit_xor minus lshift rshift rdiv
> > +	 trunc_div ceil_div floor_div round_div trunc_mod ceil_mod
> floor_mod
> > +	 round_mod)
> > + (simplify
> > +  (cond @0 (op @1 @2) (op @3 @2))
> > +   (if (!FLOAT_TYPE_P (type) || !(HONOR_NANS (@1) &&
> flag_trapping_math))
> > +    (op (cond @0 @1 @3) @2))))
> 
> Ick.  Adding a reverse tranform is going to be prone to recursing :/
> 
> Why do you need to care about NANs or FP exceptions?

Because the function in fold-const.cc has a check in the start:

  /* Do not move possibly trapping operations into the conditional as this
     pessimizes code and causes gimplification issues when applied late.  */

And so I stayed conservative and just didn't want to touch them.

> How do you know if they can't be folded?

Because otherwise they would have been reduced in fold-const.cc.
The new conditions long with the rest in fold-const.cc require:

1. at least one of the operands to be constant
2. if the operands are equal, at least one of them must have been reduced to a constant.

These two means that (cond @0 (op @1 @2) (op @3 @2)) can't match if it's something
that fold-const.cc can handle.

> Since match.pd cannot handle arbitrary operations it
> isn't a good fit for match.pd patterns, instead this would be a forwprop
> pattern or, in case you want to catch GENERIC, a fold-const.cc one?

I'll try to move it to fold-const.cc then.

Tamar

> 
> Thanks and sorry for the late reply - hope Jeffs approval didn't make you
> apply it yet.
> 
> Richard.
> 
> > +
> >  #if GIMPLE
> >  (match (nop_atomic_bit_test_and_p @0 @1 @4)
> >   (bit_and (convert?@4 (ATOMIC_FETCH_OR_XOR_N @2 INTEGER_CST@0
> @3))
> > diff --git a/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
> > b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..1d97da5c0d6454175881c2199
> 274
> > 71a567a6f0c7
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/if-compare_3.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-O3 -std=c99" } */
> > +/* { dg-final { check-function-bodies "**" "" "" { target { le } } }
> > +} */
> > +
> > +/*
> > +**zoo:
> > +**	cmp	w0, w1
> > +**	csel	w0, w3, w2, le
> > +**	ret
> > +*/
> > +int zoo (int a, int b, int c, int d)
> > +{
> > +   return a > b ? c : d;
> > +}
> > +
> > +/*
> > +**zoo1:
> > +**	cmp	w0, w1
> > +**	csel	w0, w3, w2, le
> > +**	and	w0, w0, 1
> > +**	ret
> > +*/
> > +int zoo1 (int a, int b, int c, int d) {
> > +   return (a > b ? c : d) & 1;
> > +}
> > +
> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index
> >
> b19710392940cf469de52d006603ae1e3deb6b76..aaf1b29da5c598add25dad2c
> 38b8
> > 28eaa89c49ce 100644
> > --- a/gcc/tree-cfg.cc
> > +++ b/gcc/tree-cfg.cc
> > @@ -4244,7 +4244,9 @@ verify_gimple_assign_ternary (gassign *stmt)
> >        return true;
> >      }
> >
> > -  if (!is_gimple_val (rhs1)
> > +  /* In a COND_EXPR the rhs1 is the condition and thus isn't
> > +     a gimple value by definition.  */  if ((!is_gimple_val (rhs1) &&
> > + rhs_code != COND_EXPR)
> >        || !is_gimple_val (rhs2)
> >        || !is_gimple_val (rhs3))
> >      {
> >
> >
> >
> >
> >
> 
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461
> Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald,
> Boudien Moerman; HRB 36809 (AG Nuernberg)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-10-17  9:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-23  9:21 [PATCH]middle-end fix floating out of constants in conditionals Tamar Christina
2022-09-24 20:44 ` Jeff Law
2022-09-26 10:28 ` Richard Biener
2022-09-26 11:08   ` Eric Botcazou
2022-09-26 11:12     ` Eric Botcazou
2022-10-17  9:17   ` Tamar Christina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).