Message ID | 153badc6-8afc-0695-32b2-ab5a9e0a161d@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id ACA823952522 for <patchwork@sourceware.org>; Wed, 16 Nov 2022 02:33:21 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org ACA823952522 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668566001; bh=wyNVfNXjh0mpeWgKg+nNg+8/7JcwEikPeexCfm1UeBI=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=nj7UNDZ24mB+viJ8ptQhuM9bvLf0e1ez6Q819QVLpq76aJOaoVAOJUyPmd0mraV3H 5TfOnaMtnJ9c1CJmk4WlGNnm13LbDaLmZF4E9KZ2d6c4pM02iTFIZ8di/v+SUbfzYZ 02nC+ZLVzgzZtu8DMJKstCKaMfcH24gtvLAHI1dE= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 4BE18394343C for <gcc-patches@gcc.gnu.org>; Wed, 16 Nov 2022 02:32:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4BE18394343C Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2AG2OjNE003682; Wed, 16 Nov 2022 02:32:50 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3kvq2904bs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 16 Nov 2022 02:32:50 +0000 Received: from m0098419.ppops.net (m0098419.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 2AG2Wnxk030547; Wed, 16 Nov 2022 02:32:49 GMT Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0b-001b2d01.pphosted.com (PPS) with ESMTPS id 3kvq2904be-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 16 Nov 2022 02:32:49 +0000 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 2AG2LgED004986; Wed, 16 Nov 2022 02:32:48 GMT Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by ppma04ams.nl.ibm.com with ESMTP id 3kt348w6jf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 16 Nov 2022 02:32:48 +0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 2AG2WiDZ4522550 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 16 Nov 2022 02:32:44 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9805D4C044; Wed, 16 Nov 2022 02:32:44 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 1773A4C040; Wed, 16 Nov 2022 02:32:43 +0000 (GMT) Received: from [9.200.144.161] (unknown [9.200.144.161]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 16 Nov 2022 02:32:42 +0000 (GMT) Message-ID: <153badc6-8afc-0695-32b2-ab5a9e0a161d@linux.ibm.com> Date: Wed, 16 Nov 2022 10:32:42 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 To: gcc-patches <gcc-patches@gcc.gnu.org> Cc: Segher Boessenkool <segher@kernel.crashing.org>, David <dje.gcc@gmail.com>, "Kewen.Lin" <linkw@linux.ibm.com>, Peter Bergner <bergner@linux.ibm.com> Subject: [rs6000, patch] Enable have_cbranchcc4 on rs6000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: uy4Ot7k40wPCRpXrT3kmZAaqdJYMkead X-Proofpoint-ORIG-GUID: 3SxfhAt38ew2wq-h-DWRvbcaEDm8cydN X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-11-15_08,2022-11-15_03,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 bulkscore=0 spamscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 phishscore=0 clxscore=1015 lowpriorityscore=0 priorityscore=1501 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000 definitions=main-2211160016 X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, KAM_SHORT, RCVD_IN_MSPIKE_H2, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: HAO CHEN GUI <guihaoc@linux.ibm.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[rs6000] Enable have_cbranchcc4 on rs6000
|
|
Commit Message
HAO CHEN GUI
Nov. 16, 2022, 2:32 a.m. UTC
Hi, The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to indicate if branch by CC bits is invalid or not. As rs6000 already has "*cbranch" insn which does branching according to CC bits, the flag should be enabled and relevant branches can be optimized out. The test case illustrates the optimization. "*cbranch" is an anonymous insn which can't be generated directly. So changing "const_int 0" to the third operand predicated by "zero_constant" won't cause ICEs as orginal patterns still can be matched. Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? Any recommendations? Thanks a lot. ChangeLog 2022-11-16 Haochen Gui <guihaoc@linux.ibm.com> gcc/ * config/rs6000/rs6000.md (*cbranch): Rename to... (cbranchcc4): ...this, and set const_int 0 to the third operand. gcc/testsuite/ * gcc.target/powerpc/cbranchcc4.c: New. patch.diff
Comments
On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > Hi, > The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to > indicate if branch by CC bits is invalid or not. As rs6000 already has > "*cbranch" insn which does branching according to CC bits, the flag > should be enabled and relevant branches can be optimized out. The test > case illustrates the optimization. > > "*cbranch" is an anonymous insn which can't be generated directly. > So changing "const_int 0" to the third operand predicated by > "zero_constant" won't cause ICEs as orginal patterns still can be matched. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > > ChangeLog > 2022-11-16 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * config/rs6000/rs6000.md (*cbranch): Rename to... > (cbranchcc4): ...this, and set const_int 0 to the third operand. > > gcc/testsuite/ > * gcc.target/powerpc/cbranchcc4.c: New. > > > patch.diff > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index e9e5cd1e54d..ee171f21f6a 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -13067,11 +13067,11 @@ (define_insn_and_split "*<code><mode>_cc" > ;; Conditional branches. > ;; These either are a single bc insn, or a bc around a b. > > -(define_insn "*cbranch" > +(define_insn "cbranchcc4" > [(set (pc) > (if_then_else (match_operator 1 "branch_comparison_operator" > [(match_operand 2 "cc_reg_operand" > "y") > - (const_int 0)]) > + (match_operand 3 "zero_constant")]) > (label_ref (match_operand 0)) > (pc)))] > "" > Shouldn't cbranchcc4 be a separate pattern? This pattern has an existing purpose and an expected ordering of operands. cbranchcc4 is passed the comparison operator as operand 0. Other ports ignore the second comparison operator and use (const_int 0). Operand 3 is the label, which seems to be the reason that you needed to change it to match_operand 3. It's great to add cbranchcc4 to the Power port where it definitely was an omission, but adapting *cbranch for that purpose is the wrong approach. The changes to the pattern are incorrect because they are covering up a difference in ordering of the operands. One can argue that the named pattern only enables the functionality in ifcvt and the pattern otherwise is used in its previous role. But this is a Frankenstein monster approach. You're trying to twist the existing pattern so that it triggers as cbranchcc4, but creating a pattern that messes up its arguments and only works because the new, named pattern never is called. This is too ugly. Please fix. Thanks, David diff --git a/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c > b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c > new file mode 100644 > index 00000000000..1751d274bbf > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-rtl-ce1" } */ > +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */ > + > +int test (unsigned int a, unsigned int b) > +{ > + return (a < b ? 0 : (a > b ? 2 : 1)); > +} >
Hi David,
I found definition of the operands in 'cbranch'. The argumnets matters.
I will create a new expand pattern for cbranchcc4. Thanks a lot for your
comments.
'cbranchmode4’
Conditional branch instruction combined with a compare instruction.
Operand 0 is a comparison operator. Operand 1 and operand 2 are the
first and second operands of the comparison, respectively. Operand 3
is the code_label to jump to.
Gui Haochen
Thanks
在 2022/11/16 11:04, David Edelsohn 写道:
> It's great to add cbranchcc4 to the Power port where it definitely was an omission, but adapting *cbranch for that purpose is the wrong approach. The changes to the pattern are incorrect because they are covering up a difference in ordering of the operands. One can argue that the named pattern only enables the functionality in ifcvt and the pattern otherwise is used in its previous role. But this is a Frankenstein monster approach. You're trying to twist the existing pattern so that it triggers as cbranchcc4, but creating a pattern that messes up its arguments and only works because the new, named pattern never is called. This is too ugly. Please fix.
Hi guys, On Tue, Nov 15, 2022 at 10:04:04PM -0500, David Edelsohn wrote: > On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > > The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to > > indicate if branch by CC bits is invalid or not. As rs6000 already has > > "*cbranch" insn which does branching according to CC bits, the flag > > should be enabled and relevant branches can be optimized out. The test > > case illustrates the optimization. cbranch<mode>4 normally compares the <mode>mode operands 2 and 3, and jumps to operands[0] if operands[1] (a relation of operands 2 and 3) is true. See emit_cmp_and_jump_insns for example. But cbranchcc4 does not do any comparison; instead, it branches based on the result of a previous comparison, stored in operands[2], and operands[3] has to be const_int 0. This really should never have been done this way, because now almost everywhere we need exception code for MODE_CC things here (and always do only the exceptional case on targets that have no use for actual compare-and-branch patterns). Another appearance of this (last stage 4!) was PR104335. We never followed up on that, probably should have, sigh. So, cbranch<mode>4 now means "compare and branch", except when <mode> is cc, in which case it means "conditional branch", and operands[3] is required to be const_int 0 in that case, and if not we ICE, or worse, do the wrong thing. Woohoo. > > "*cbranch" is an anonymous insn which can't be generated directly. It can be, rs6000_emit_cbranch (which does mean "conditional branch" btw). All RTL can be created out of thin air. But you mean "can't be generated by name"? Yup. > > So changing "const_int 0" to the third operand predicated by > > "zero_constant" won't cause ICEs as orginal patterns still can be matched. It never is valid to have an actual comparison of anything MODE_CC with anything else, only MODE_INT, MODE_FLOAT, MODE_DECIMAL_FLOAT have a (total) order. The (const_int 0) in such MODE_CC RTL is only notational convenience, it does not mean integer 0 or anything else, it is a placeholder so things can look like a relation. > > -(define_insn "*cbranch" > > +(define_insn "cbranchcc4" > > [(set (pc) > > (if_then_else (match_operator 1 "branch_comparison_operator" > > [(match_operand 2 "cc_reg_operand" "y") > > - (const_int 0)]) > > + (match_operand 3 "zero_constant")]) > > (label_ref (match_operand 0)) > > (pc)))] > > "" > Shouldn't cbranchcc4 be a separate pattern? This pattern has an existing > purpose and an expected ordering of operands. It still has the same operands, only operands[3] is added. Nothing can create the old pattern by name (because its name is starred), so the expander won't ever access operands[3] while there are only 3 operands passed -- simply because the expander cannot be called in any way :-) > cbranchcc4 is passed the comparison operator as operand 0. Other ports > ignore the second comparison operator and use (const_int 0). Operand 3 is > the label, which seems to be the reason that you needed to change it to > match_operand 3. operands[0] is the dest, operands[1] is the relation, operands[2] is the cc reg, operands[3] is const_int 0. The dest is last in the pattern, to make it more confusing to read :-) > It's great to add cbranchcc4 to the Power port where it definitely was an > omission, but adapting *cbranch for that purpose is the wrong approach. > The changes to the pattern are incorrect because they are covering up a > difference in ordering of the operands. One can argue that the named > pattern only enables the functionality in ifcvt and the pattern otherwise > is used in its previous role. But this is a Frankenstein monster > approach. I agree with that! But the generic code adopted that, and other ports followed it as well, so who are we to disdain this nice performance improvement? > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c > > @@ -0,0 +1,8 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -fdump-rtl-ce1" } */ > > +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */ (space after { please? For consistency.) > > +int test (unsigned int a, unsigned int b) > > +{ > > + return (a < b ? 0 : (a > b ? 2 : 1)); > > +} Please add a comment what this test wants to test for? Something like /* If-conversion should <something here>. */ maybe? I would approve this patch, given that the compiler will immediately ICE if something not a zero is passed as operands[3] (although it would be nice to have a zero_int_operand that would aonly allow const_int). David, do you agree given all of the above? Thanks, Segher
On Wed, Nov 16, 2022 at 06:06:19AM -0600, Segher Boessenkool wrote: > Hi guys, > > On Tue, Nov 15, 2022 at 10:04:04PM -0500, David Edelsohn wrote: > > On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > > > The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to > > > indicate if branch by CC bits is invalid or not. As rs6000 already has > > > "*cbranch" insn which does branching according to CC bits, the flag > > > should be enabled and relevant branches can be optimized out. The test > > > case illustrates the optimization. > > cbranch<mode>4 normally compares the <mode>mode operands 2 and 3, and > jumps to operands[0] if operands[1] (a relation of operands 2 and 3) is > true. See emit_cmp_and_jump_insns for example. But cbranchcc4 does > not do any comparison; instead, it branches based on the result of a > previous comparison, stored in operands[2], and operands[3] has to be > const_int 0. Apparently the operand order is different? Where is that documented at all anyway? Grr. Please adjust my comments to taste :-/ Segher
Hao cbranchcc4 is a named pattern and requires a specific operand ordering. If you change *cbranch to cbranchcc4, you must change the order of the operands, not a quick and dirty hack to *cbranch. Also, you should change *cbranch_2insn and *creturn as well so that all of the patterns are consistent. See for example the aarch64.md implementation. and the documentation in Standard Names https://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html which mentions cbranch<mode>4 and, briefly, cbranchcc4. You seemed to want to make the minimal change so that the pattern would work with ifcvt without considering the impact on the existing pattern and without understanding what a named pattern with specific operands really means. You changed the pattern predicate so that the operands in the wrong positions would match the pattern. Thanks, David On Wed, Nov 16, 2022 at 12:56 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > Hi David, > I found definition of the operands in 'cbranch'. The argumnets matters. > I will create a new expand pattern for cbranchcc4. Thanks a lot for your > comments. > > 'cbranchmode4’ > Conditional branch instruction combined with a compare instruction. > Operand 0 is a comparison operator. Operand 1 and operand 2 are the > first and second operands of the comparison, respectively. Operand 3 > is the code_label to jump to. > > Gui Haochen > Thanks > > 在 2022/11/16 11:04, David Edelsohn 写道: > > It's great to add cbranchcc4 to the Power port where it definitely was > an omission, but adapting *cbranch for that purpose is the wrong approach. > The changes to the pattern are incorrect because they are covering up a > difference in ordering of the operands. One can argue that the named > pattern only enables the functionality in ifcvt and the pattern otherwise > is used in its previous role. But this is a Frankenstein monster > approach. You're trying to twist the existing pattern so that it triggers > as cbranchcc4, but creating a pattern that messes up its arguments and only > works because the new, named pattern never is called. This is too ugly. > Please fix. >
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index e9e5cd1e54d..ee171f21f6a 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -13067,11 +13067,11 @@ (define_insn_and_split "*<code><mode>_cc" ;; Conditional branches. ;; These either are a single bc insn, or a bc around a b. -(define_insn "*cbranch" +(define_insn "cbranchcc4" [(set (pc) (if_then_else (match_operator 1 "branch_comparison_operator" [(match_operand 2 "cc_reg_operand" "y") - (const_int 0)]) + (match_operand 3 "zero_constant")]) (label_ref (match_operand 0)) (pc)))] "" diff --git a/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c new file mode 100644 index 00000000000..1751d274bbf --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-rtl-ce1" } */ +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */ + +int test (unsigned int a, unsigned int b) +{ + return (a < b ? 0 : (a > b ? 2 : 1)); +}