From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 8EA0D3858D32 for ; Mon, 26 Sep 2022 10:28:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8EA0D3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 7C51B1FA3B; Mon, 26 Sep 2022 10:28:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1664188101; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hBk/tNrV+4UcR9/XyeqC77JT1jj62NP42DBIlLwJeWc=; b=Fvr/NMJWWPNWD7ok/IvnyC6wTyt7CT86y3B9NrI9lQGabjA/p4QT/T6Q7N3FRNJRjyrzBX usXKwVwlPH2sGfTCGK1myilBZXGx3oaoJnT78aOWIZzeTsGIBm1LwbSzAeIryaMtDQ4YLf Cab4JUiXFgyHAXsNjrXbD2YZucwWip8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1664188101; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hBk/tNrV+4UcR9/XyeqC77JT1jj62NP42DBIlLwJeWc=; b=qRTWxDHeoqDkIOXl1vAPWx8GY7Xa+OiRg51S6s7SOHLFbAga0vF/xhvATu6gs5lblrz4sM 6LrILRK5zL6gXNAQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 71CC22C14F; Mon, 26 Sep 2022 10:28:21 +0000 (UTC) Date: Mon, 26 Sep 2022 10:28:21 +0000 (UTC) From: Richard Biener To: Tamar Christina cc: gcc-patches@gcc.gnu.org, nd@arm.com, jeffreyalaw@gmail.com, ebotcazou@adacore.com Subject: Re: [PATCH]middle-end fix floating out of constants in conditionals In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_LOTSOFHASH,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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 SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)