> -----Original Message----- > From: Richard Biener <rguent...@suse.de> > Sent: Monday, September 26, 2022 11:28 AM > To: Tamar Christina <tamar.christ...@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; jeffreya...@gmail.com; > ebotca...@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 <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)