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 <rguent...@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)
