On Fri, Sep 20, 2024 at 3:07 AM Andrew Pinski <quic_apin...@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_apin...@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
>
