diff mbox series

match: Fix `a != 0 ? a * b : 0` patterns for things that trap [PR116772]

Message ID 20240920010621.3217134-1-quic_apinski@quicinc.com
State New
Headers show
Series match: Fix `a != 0 ? a * b : 0` patterns for things that trap [PR116772] | expand

Commit Message

Andrew Pinski Sept. 20, 2024, 1:06 a.m. UTC 
For generic, `a != 0 ? a * b : 0` would match where `b` would be an expression
which trap (in the case of the testcase, it was an integer division but it could be any).

This fixes the issue by adding a condition for `(a != 0 ? expr : 0)` to check for expressions
which have side effects or traps.

	PR middle-end/116772

gcc/ChangeLog:

	* match.pd (`a != 0 ? a / b : 0`): Add a check to make
	sure b does not trap or have side effects.
	(`a != 0 ? a * b : 0`, `a != 0 ? a & b : 0`): Likewise.

gcc/testsuite/ChangeLog:

	* gcc.dg/torture/pr116772-1.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/match.pd                              | 12 ++++++++++--
 gcc/testsuite/gcc.dg/torture/pr116772-1.c | 24 +++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr116772-1.c

Comments

Richard Biener Sept. 20, 2024, 6:20 a.m. UTC | #1 
On Fri, Sep 20, 2024 at 3:07 AM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> For generic, `a != 0 ? a * b : 0` would match where `b` would be an expression
> which trap (in the case of the testcase, it was an integer division but it could be any).
>
> This fixes the issue by adding a condition for `(a != 0 ? expr : 0)` to check for expressions
> which have side effects or traps.

I think the better fix is to restrict the patterns to GIMPLE - it
doesn't look like they were
moved over from fold-const.cc?  Another option might be to have a way to check
that @3 and @1 are "leaf" (aka non-EXPRs and non-REFERENCEs).

If you think the issue could be more wide-spread and we want to preserve the
folding at GENERIC (it might be useful for SCEV or niter analysis both which
eventually add COND_EXPRs...), then can you, instead of repeating

> +       && !generic_expr_could_trap_p (@3)
> +       && (GIMPLE || !TREE_SIDE_EFFECTS (@3)))

introduce an inline function in {gimple,generic}-match-head.cc for this,
say

static inline bool
no_side_effects (tree t)
{
   return !TREE_SIDE_EFFECTS (t) && !generic_expr_could_trap_p (t);
}

and on the GIMPLE side return true (and checking-assert we have a
is_gimple_val).

Thanks,
Richard.

>         PR middle-end/116772
>
> gcc/ChangeLog:
>
>         * match.pd (`a != 0 ? a / b : 0`): Add a check to make
>         sure b does not trap or have side effects.
>         (`a != 0 ? a * b : 0`, `a != 0 ? a & b : 0`): Likewise.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/torture/pr116772-1.c: New test.
>
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
>  gcc/match.pd                              | 12 ++++++++++--
>  gcc/testsuite/gcc.dg/torture/pr116772-1.c | 24 +++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr116772-1.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index fdb59ff0d44..db46f319c5f 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -4663,7 +4663,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>   (simplify
>    (cond (ne @0 integer_zerop) (op@2 @3 @1) integer_zerop )
>     (if (bitwise_equal_p (@0, @3)
> -        && tree_expr_nonzero_p (@1))
> +        && tree_expr_nonzero_p (@1)
> +       /* Cannot make a trapping expression or with one with side
> +          effects unconditional. */
> +       && !generic_expr_could_trap_p (@3)
> +       && (GIMPLE || !TREE_SIDE_EFFECTS (@3)))
>      @2)))
>
>  /* Note we prefer the != case here
> @@ -4673,7 +4677,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  (for op (mult bit_and)
>   (simplify
>    (cond (ne @0 integer_zerop) (op:c@2 @1 @3) integer_zerop)
> -  (if (bitwise_equal_p (@0, @3))
> +  (if (bitwise_equal_p (@0, @3)
> +       /* Cannot make a trapping expression or with one with side
> +          effects unconditional. */
> +       && !generic_expr_could_trap_p (@1)
> +       && (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
>     @2)))
>
>  /* Simplifications of shift and rotates.  */
> diff --git a/gcc/testsuite/gcc.dg/torture/pr116772-1.c b/gcc/testsuite/gcc.dg/torture/pr116772-1.c
> new file mode 100644
> index 00000000000..eedd0398af1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr116772-1.c
> @@ -0,0 +1,24 @@
> +/* { dg-do run } */
> +/* PR middle-end/116772  */
> +/* The division by `/b` should not
> +   be made uncondtional. */
> +
> +int mult0(int a,int b) __attribute__((noipa));
> +
> +int mult0(int a,int b){
> +  return (b!=0 ? (a/b)*b : 0);
> +}
> +
> +int bit_and0(int a,int b) __attribute__((noipa));
> +
> +int bit_and0(int a,int b){
> +  return (b!=0 ? (a/b)&b : 0);
> +}
> +
> +int main() {
> +  if (mult0(3, 0) != 0)
> +    __builtin_abort();
> +  if (bit_and0(3, 0) != 0)
> +    __builtin_abort();
> +  return 0;
> +}
> --
> 2.43.0
>
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index fdb59ff0d44..db46f319c5f 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -4663,7 +4663,11 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  (simplify
   (cond (ne @0 integer_zerop) (op@2 @3 @1) integer_zerop )
    (if (bitwise_equal_p (@0, @3)
-        && tree_expr_nonzero_p (@1))
+        && tree_expr_nonzero_p (@1)
+	/* Cannot make a trapping expression or with one with side
+	   effects unconditional. */
+	&& !generic_expr_could_trap_p (@3)
+	&& (GIMPLE || !TREE_SIDE_EFFECTS (@3)))
     @2)))
 
 /* Note we prefer the != case here
@@ -4673,7 +4677,11 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 (for op (mult bit_and)
  (simplify
   (cond (ne @0 integer_zerop) (op:c@2 @1 @3) integer_zerop)
-  (if (bitwise_equal_p (@0, @3))
+  (if (bitwise_equal_p (@0, @3)
+	/* Cannot make a trapping expression or with one with side
+	   effects unconditional. */
+	&& !generic_expr_could_trap_p (@1)
+	&& (GIMPLE || !TREE_SIDE_EFFECTS (@1)))
    @2)))
 
 /* Simplifications of shift and rotates.  */
diff --git a/gcc/testsuite/gcc.dg/torture/pr116772-1.c b/gcc/testsuite/gcc.dg/torture/pr116772-1.c
new file mode 100644
index 00000000000..eedd0398af1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr116772-1.c
@@ -0,0 +1,24 @@ 
+/* { dg-do run } */
+/* PR middle-end/116772  */
+/* The division by `/b` should not
+   be made uncondtional. */
+
+int mult0(int a,int b) __attribute__((noipa));
+
+int mult0(int a,int b){
+  return (b!=0 ? (a/b)*b : 0);
+}
+
+int bit_and0(int a,int b) __attribute__((noipa));
+
+int bit_and0(int a,int b){
+  return (b!=0 ? (a/b)&b : 0);
+}
+
+int main() {
+  if (mult0(3, 0) != 0)
+    __builtin_abort();
+  if (bit_and0(3, 0) != 0)
+    __builtin_abort();
+  return 0;
+}