From: Tamar Christina <Tamar.Christina@arm.com>
To: Richard Biener <rguenther@suse.de>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>, "jeffreyalaw@gmail.com" <jeffreyalaw@gmail.com>,
"ebotcazou@adacore.com" <ebotcazou@adacore.com>
Subject: RE: [PATCH]middle-end fix floating out of constants in conditionals
Date: Mon, 17 Oct 2022 09:17:26 +0000 [thread overview]
Message-ID: <VI1PR08MB5325CCA7C410687748C9A209FF299@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <nycvar.YFH.7.77.849.2209261016380.6652@jbgna.fhfr.qr>
> -----Original Message-----
> From: Richard Biener <rguenther@suse.de>
> Sent: Monday, September 26, 2022 11:28 AM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; jeffreyalaw@gmail.com;
> ebotcazou@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 <rguenther@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)
prev parent reply other threads:[~2022-10-17 9:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-23 9:21 Tamar Christina
2022-09-24 20:44 ` Jeff Law
2022-09-26 10:28 ` Richard Biener
2022-09-26 11:08 ` Eric Botcazou
2022-09-26 11:12 ` Eric Botcazou
2022-10-17 9:17 ` Tamar Christina [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR08MB5325CCA7C410687748C9A209FF299@VI1PR08MB5325.eurprd08.prod.outlook.com \
--to=tamar.christina@arm.com \
--cc=ebotcazou@adacore.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jeffreyalaw@gmail.com \
--cc=nd@arm.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).