diff mbox series

MATCH: Add some more value_replacement simplifications (a != 0 ? expr : 0) to match

Message ID 20240501032114.1112795-1-quic_apinski@quicinc.com
State Committed
Headers
Series MATCH: Add some more value_replacement simplifications (a != 0 ? expr : 0) to match |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

Andrew Pinski May 1, 2024, 3:21 a.m. UTC 
  This adds a few more of what is currently done in phiopt's value_replacement
to match. I noticed this when I was hooking up phiopt's value_replacement
code to use match and disabling the old code. But this can be done
independently from the hooking up phiopt's value_replacement as phiopt
is already hooked up for simplified versions already.

/* a != 0 ? a / b : 0  -> a / b iff b is nonzero. */
/* a != 0 ? a * b : 0 -> a * b */
/* a != 0 ? a & b : 0 -> a & b */

We prefer the `cond ? a : 0` forms to allow optimization of `a * cond` which
uses that form.

Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR treee-optimization/114894

gcc/ChangeLog:

	* match.pd (`a != 0 ? a / b : 0`): New pattern.
	(`a != 0 ? a * b : 0`): New pattern.
	(`a != 0 ? a & b : 0`): New pattern.

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/phi-opt-value-5.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/match.pd                                  | 18 +++++++++
 .../gcc.dg/tree-ssa/phi-opt-value-5.c         | 39 +++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-value-5.c

Comments

Jeff Law May 7, 2024, 8:44 p.m. UTC | #1 
On 4/30/24 9:21 PM, Andrew Pinski wrote:
> This adds a few more of what is currently done in phiopt's value_replacement
> to match. I noticed this when I was hooking up phiopt's value_replacement
> code to use match and disabling the old code. But this can be done
> independently from the hooking up phiopt's value_replacement as phiopt
> is already hooked up for simplified versions already.
> 
> /* a != 0 ? a / b : 0  -> a / b iff b is nonzero. */
> /* a != 0 ? a * b : 0 -> a * b */
> /* a != 0 ? a & b : 0 -> a & b */
> 
> We prefer the `cond ? a : 0` forms to allow optimization of `a * cond` which
> uses that form.
> 
> Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> 
> 	PR treee-optimization/114894
> 
> gcc/ChangeLog:
> 
> 	* match.pd (`a != 0 ? a / b : 0`): New pattern.
> 	(`a != 0 ? a * b : 0`): New pattern.
> 	(`a != 0 ? a & b : 0`): New pattern.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/tree-ssa/phi-opt-value-5.c: New test.
Is there any need to also handle the reversed conditional with the arms 
swapped?    If not, this is fine as-is.  If yes, then fine with the 
obvious generalization.

jeff
Andrew Pinski May 7, 2024, 8:55 p.m. UTC | #2 
On Tue, May 7, 2024 at 1:45 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 4/30/24 9:21 PM, Andrew Pinski wrote:
> > This adds a few more of what is currently done in phiopt's value_replacement
> > to match. I noticed this when I was hooking up phiopt's value_replacement
> > code to use match and disabling the old code. But this can be done
> > independently from the hooking up phiopt's value_replacement as phiopt
> > is already hooked up for simplified versions already.
> >
> > /* a != 0 ? a / b : 0  -> a / b iff b is nonzero. */
> > /* a != 0 ? a * b : 0 -> a * b */
> > /* a != 0 ? a & b : 0 -> a & b */
> >
> > We prefer the `cond ? a : 0` forms to allow optimization of `a * cond` which
> > uses that form.
> >
> > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> >       PR treee-optimization/114894
> >
> > gcc/ChangeLog:
> >
> >       * match.pd (`a != 0 ? a / b : 0`): New pattern.
> >       (`a != 0 ? a * b : 0`): New pattern.
> >       (`a != 0 ? a & b : 0`): New pattern.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.dg/tree-ssa/phi-opt-value-5.c: New test.
> Is there any need to also handle the reversed conditional with the arms
> swapped?    If not, this is fine as-is.  If yes, then fine with the
> obvious generalization.

The answer is yes and no. While the PHI-OPT pass will try both cases
but the other (all?) passes does not. This is something I have been
thinking about trying to solve in a generic way instead of adding many
more patterns here. I will start working on that in the middle of
June.
Most of the time cond patterns in match are used is inside phiopt so
having the revered conditional has not been on high on my priority but
with VRP and scev and match (itself) producing more cond_expr, we
should fix this once and for all for GCC 15.

Thanks,
Andrew Pinski

>
> jeff
>
Richard Biener May 8, 2024, 7:43 a.m. UTC | #3 
On Tue, May 7, 2024 at 10:56 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Tue, May 7, 2024 at 1:45 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >
> >
> >
> > On 4/30/24 9:21 PM, Andrew Pinski wrote:
> > > This adds a few more of what is currently done in phiopt's value_replacement
> > > to match. I noticed this when I was hooking up phiopt's value_replacement
> > > code to use match and disabling the old code. But this can be done
> > > independently from the hooking up phiopt's value_replacement as phiopt
> > > is already hooked up for simplified versions already.
> > >
> > > /* a != 0 ? a / b : 0  -> a / b iff b is nonzero. */
> > > /* a != 0 ? a * b : 0 -> a * b */
> > > /* a != 0 ? a & b : 0 -> a & b */
> > >
> > > We prefer the `cond ? a : 0` forms to allow optimization of `a * cond` which
> > > uses that form.
> > >
> > > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > >
> > >       PR treee-optimization/114894
> > >
> > > gcc/ChangeLog:
> > >
> > >       * match.pd (`a != 0 ? a / b : 0`): New pattern.
> > >       (`a != 0 ? a * b : 0`): New pattern.
> > >       (`a != 0 ? a & b : 0`): New pattern.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >       * gcc.dg/tree-ssa/phi-opt-value-5.c: New test.
> > Is there any need to also handle the reversed conditional with the arms
> > swapped?    If not, this is fine as-is.  If yes, then fine with the
> > obvious generalization.
>
> The answer is yes and no. While the PHI-OPT pass will try both cases
> but the other (all?) passes does not. This is something I have been
> thinking about trying to solve in a generic way instead of adding many
> more patterns here. I will start working on that in the middle of
> June.
> Most of the time cond patterns in match are used is inside phiopt so
> having the revered conditional has not been on high on my priority but
> with VRP and scev and match (itself) producing more cond_expr, we
> should fix this once and for all for GCC 15.

IMO this is a classical case for canonicalization.  IIRC in fold we
rely on tree_swap_operands_p for the COND_EXPR arms and if
we can invert the condition we do so.  So there's a conflict of interest
with respect to condition canonicalization and true/false canonicalization.
We do not canonicalize COND_EXPRs in gimple_resimplify3, but
the only natural thing there would be to do it based on the op2/op3
operands, looking at the conditional would dive down one level too deep.

Richard.

> Thanks,
> Andrew Pinski
>
> >
> > jeff
> >
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index d401e7503e6..03a03c31233 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4290,6 +4290,24 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
   (cond (eq @0 integer_all_onesp) @1 (op:c@2 @1 @0))
    @2))
 
+/* a != 0 ? a / b : 0  -> a / b iff b is nonzero. */
+(for op (trunc_div ceil_div floor_div round_div exact_div)
+ (simplify
+  (cond (ne @0 integer_zerop) (op@2 @3 @1) integer_zerop )
+   (if (bitwise_equal_p (@0, @3)
+        && tree_expr_nonzero_p (@1))
+    @2)))
+
+/* Note we prefer the != case here
+   as (a != 0) * (a * b) will generate that version. */
+/* a != 0 ? a * b : 0 -> a * b */
+/* a != 0 ? a & b : 0 -> a & b */
+(for op (mult bit_and)
+ (simplify
+  (cond (ne @0 integer_zerop) (op:c@2 @1 @3) integer_zerop)
+  (if (bitwise_equal_p (@0, @3))
+   @2)))
+
 /* Simplifications of shift and rotates.  */
 
 (for rotate (lrotate rrotate)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-value-5.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-value-5.c
new file mode 100644
index 00000000000..8062eb19b11
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-value-5.c
@@ -0,0 +1,39 @@ 
+/* { dg-do compile } */
+/* PR treee-optimization/114894 */
+/* Phi-OPT should be able to optimize these without sinking being invoked. */
+/* { dg-options "-O -fdump-tree-phiopt2 -fdump-tree-phiopt3 -fdump-tree-optimized -fno-tree-sink" } */
+
+int fmul1(int a, int b)
+{
+  int c = a * b;
+  if (a != 0)
+    return c;
+  return 0;
+}
+
+
+int fand1(int a, int b)
+{
+  int c = a & b;
+  if (a != 0)
+    return c;
+  return 0;
+}
+
+
+void g(int);
+
+int fdiv1(int a, int b)
+{
+  int d = b|1;
+  g(d);
+  int c = a / d;
+  return a != 0 ? c : 0;
+}
+
+/* fdiv1 requires until later than phiopt2 to be able to detect that
+   d is non-zero. to be able to remove the conditional.  */
+/* { dg-final { scan-tree-dump-times "goto" 2 "phiopt2" } } */
+/* { dg-final { scan-tree-dump-not "goto" "phiopt3" } } */
+/* { dg-final { scan-tree-dump-not "goto" "optimized" } } */
+