Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
Message ID | 4a052a62-7861-ed6f-9801-3b58ac384f81@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 65599385703A for <patchwork@sourceware.org>; Wed, 23 Nov 2022 02:55:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 65599385703A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1669172124; bh=KlNf2fpRvoPsOSUv25H6fZ2ThNdWKdIWlhEPgYCEAfg=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=GLb7xKcwT55y1VYFyGdc3IryZ/6ulieYxcpDnYX6haS/RudXcLyM6BXDW/048Xgwn 8ogvZG2esR9oDxu2eSandbMTvwdSDQ+xzMhke86d/mx//IjxLtzekrWhJKF/rCr4U2 Ygv55D8wbywA7VvgrowLEmfdSIUD7BJQZ5GcZEEc= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 3EE3C38582BC for <gcc-patches@gcc.gnu.org>; Wed, 23 Nov 2022 02:54:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3EE3C38582BC Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2AN20REh009812; Wed, 23 Nov 2022 02:54:51 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3m10w5estj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 23 Nov 2022 02:54:51 +0000 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 2AN2kP6l015384; Wed, 23 Nov 2022 02:54:51 GMT Received: from ppma06ams.nl.ibm.com (66.31.33a9.ip4.static.sl-reverse.com [169.51.49.102]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3m10w5esst-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 23 Nov 2022 02:54:50 +0000 Received: from pps.filterd (ppma06ams.nl.ibm.com [127.0.0.1]) by ppma06ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 2AN2qYcZ017637; Wed, 23 Nov 2022 02:54:48 GMT Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by ppma06ams.nl.ibm.com with ESMTP id 3kxpdhw1h4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 23 Nov 2022 02:54:48 +0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 2AN2sjD866257290 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 23 Nov 2022 02:54:45 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 75FE54C04A; Wed, 23 Nov 2022 02:54:45 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id ED9994C046; Wed, 23 Nov 2022 02:54:43 +0000 (GMT) Received: from [9.200.144.161] (unknown [9.200.144.161]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 23 Nov 2022 02:54:43 +0000 (GMT) Message-ID: <4a052a62-7861-ed6f-9801-3b58ac384f81@linux.ibm.com> Date: Wed, 23 Nov 2022 10:54:42 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 To: gcc-patches <gcc-patches@gcc.gnu.org> Cc: Segher Boessenkool <segher@kernel.crashing.org>, David <dje.gcc@gmail.com>, Peter Bergner <bergner@linux.ibm.com>, "Kewen.Lin" <linkw@linux.ibm.com> Subject: [PATCH] Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: 6Q95l3kaVA1lVdzAIFutXLNGANoPdyVG X-Proofpoint-ORIG-GUID: 8WubkJNlp9K41JThqEAs676uFZIjhmix 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-22_13,2022-11-18_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 malwarescore=0 bulkscore=0 adultscore=0 mlxscore=0 lowpriorityscore=0 suspectscore=0 impostorscore=0 priorityscore=1501 spamscore=0 phishscore=0 mlxlogscore=944 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000 definitions=main-2211230017 X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, 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 |
Change the behavior of predicate check failure on cbranchcc4 operand0 in prepare_cmp_insn
|
|
Commit Message
HAO CHEN GUI
Nov. 23, 2022, 2:54 a.m. UTC
Hi, I want to enable "have_cbranchcc4" on rs6000. But not all combinations of comparison codes and sub CC modes are benefited to generate cbranchcc4 insns on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass some combinations. It gets assertion failure in prepare_cmp_insn. I think we shouldn't suppose that all comparison codes and sub CC modes are supported and throw an assertion failure in prepare_cmp_insn. It might check the predicate and go to fail if the predicate can't be satisfied. This patch changes the behavior of those codes. Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions. Is this okay for trunk? Any recommendations? Thanks a lot. ChangeLog 2022-11-23 Haochen Gui <guihaoc@linux.ibm.com> gcc/ * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when predicate check of "cbranchcc4" operand[0] fails. patch.diff
Comments
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html Thanks Gui Haochen 在 2022/11/23 10:54, HAO CHEN GUI 写道: > Hi, > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > some combinations. It gets assertion failure in prepare_cmp_insn. I think > we shouldn't suppose that all comparison codes and sub CC modes are supported > and throw an assertion failure in prepare_cmp_insn. It might check the > predicate and go to fail if the predicate can't be satisfied. This patch > changes the behavior of those codes. > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > > ChangeLog > 2022-11-23 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > predicate check of "cbranchcc4" operand[0] fails. > > patch.diff > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > index 165f8d1fa22..3ec8f6b17ba 100644 > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size, > { > enum insn_code icode = optab_handler (cbranch_optab, CCmode); > test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > - gcc_assert (icode != CODE_FOR_nothing > - && insn_operand_matches (icode, 0, test)); > + gcc_assert (icode != CODE_FOR_nothing); > + if (!insn_operand_matches (icode, 0, test)) > + goto fail; > *ptest = test; > return; > }
Add more experts in CC. on 2022/11/23 10:54, HAO CHEN GUI wrote: > Hi, > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > some combinations. It gets assertion failure in prepare_cmp_insn. I think > we shouldn't suppose that all comparison codes and sub CC modes are supported > and throw an assertion failure in prepare_cmp_insn. It might check the > predicate and go to fail if the predicate can't be satisfied. This patch > changes the behavior of those codes. > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > > ChangeLog > 2022-11-23 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > predicate check of "cbranchcc4" operand[0] fails. > > patch.diff > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > index 165f8d1fa22..3ec8f6b17ba 100644 > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size, > { > enum insn_code icode = optab_handler (cbranch_optab, CCmode); > test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > - gcc_assert (icode != CODE_FOR_nothing > - && insn_operand_matches (icode, 0, test)); > + gcc_assert (icode != CODE_FOR_nothing); > + if (!insn_operand_matches (icode, 0, test)) > + goto fail; IMHO, this change looks to accord with the other code in prepare_cmp_insn, which allows the preparation to fail with NULL_RTX ptest. Its caller can make its own decision (ICE due to unexpected, or try other ways) when ptest is null. If this direction is sensible, maybe we can make it goto fail too if the icode == CODE_FOR_nothing, since we already try to relax the restriction. BR, Kewen > *ptest = test; > return; > }
On Mon, Nov 28, 2022 at 6:33 AM HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > Gentle ping this: > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607083.html > Thanks > Gui Haochen > > 在 2022/11/23 10:54, HAO CHEN GUI 写道: > > Hi, > > I want to enable "have_cbranchcc4" on rs6000. But not all combinations of > > comparison codes and sub CC modes are benefited to generate cbranchcc4 insns > > on rs6000. There is an predicate for operand0 of cbranchcc4 to bypass > > some combinations. It gets assertion failure in prepare_cmp_insn. I think > > we shouldn't suppose that all comparison codes and sub CC modes are supported > > and throw an assertion failure in prepare_cmp_insn. It might check the > > predicate and go to fail if the predicate can't be satisfied. This patch > > changes the behavior of those codes. > > > > Bootstrapped and tested on powerpc64-linux BE/LE and x86 with no regressions. > > Is this okay for trunk? Any recommendations? Thanks a lot. Since the function seems to be allowed to fail the patch looks reasonable - still I wonder what the "fallback" for a MODE_CC style compare-and-branch is? There are callers of this function that do not seem to expect failure at least, some suspiciously looking like MODE_CC candiates. Richard. > > > > > > ChangeLog > > 2022-11-23 Haochen Gui <guihaoc@linux.ibm.com> > > > > gcc/ > > * optabs.cc (prepare_cmp_insn): Go to fail other than assert it when > > predicate check of "cbranchcc4" operand[0] fails. > > > > patch.diff > > diff --git a/gcc/optabs.cc b/gcc/optabs.cc > > index 165f8d1fa22..3ec8f6b17ba 100644 > > --- a/gcc/optabs.cc > > +++ b/gcc/optabs.cc > > @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size, > > { > > enum insn_code icode = optab_handler (cbranch_optab, CCmode); > > test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); > > - gcc_assert (icode != CODE_FOR_nothing > > - && insn_operand_matches (icode, 0, test)); > > + gcc_assert (icode != CODE_FOR_nothing); > > + if (!insn_operand_matches (icode, 0, test)) > > + goto fail; > > *ptest = test; > > return; > > }
On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote: > Since the function seems to be allowed to fail the patch looks > reasonable - still I wonder > what the "fallback" for a MODE_CC style compare-and-branch is? There > are callers > of this function that do not seem to expect failure at least, some > suspiciously looking > like MODE_CC candiates. Hi! cbranchcc4 is *not* a compare-and-branch, like ccbranch<mode>4 for other modes are. Instead, it is a conditional branch. I still think it is a bad idea to use this same pattern name for a completely different (and much more basic) concept, it just confuses many things, makes us need exceptions in most users of cbranch<mode>4 :-( cbranchcc4 does not do a comparison. Instead, it uses the result of some previous comparison in some CC register (or anything else that set such a register). We want to use a cbranchcc4 to reuse some earlier comparison here. Which is great of course! But, redoing the (potentially expensive) computation to prepare the CC for a more complicated condition is not a good idea. Also, Power's conditional branch insns just branch on one of the 32 condition bits (either set or unset), not on a logical combination of multiple of those bits, as we need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath. So it is much cleaner (and causes fewer problems later on) if we only allow those codes we do support. Example of LTGT: fcmpu 0,0,1 # compare f0 <=> f1 to cr0 (exactly one of # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set) cror 2,0,1 # cr0.eq = cr0.lt | cr0.gt beq 0 # branch if cr0.eq is set So, we want the cbranchcc4 here to just do that last insn, not the last two insns (or all three as any other cbranch<mode>4 is!) Segher
On Mon, Nov 28, 2022 at 6:58 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Mon, Nov 28, 2022 at 09:42:05AM +0100, Richard Biener wrote: > > Since the function seems to be allowed to fail the patch looks > > reasonable - still I wonder > > what the "fallback" for a MODE_CC style compare-and-branch is? There > > are callers > > of this function that do not seem to expect failure at least, some > > suspiciously looking > > like MODE_CC candiates. > > Hi! > > cbranchcc4 is *not* a compare-and-branch, like ccbranch<mode>4 for other > modes are. Instead, it is a conditional branch. I still think it is a > bad idea to use this same pattern name for a completely different (and > much more basic) concept, it just confuses many things, makes us need > exceptions in most users of cbranch<mode>4 :-( > > cbranchcc4 does not do a comparison. Instead, it uses the result of > some previous comparison in some CC register (or anything else that set > such a register). We want to use a cbranchcc4 to reuse some earlier > comparison here. Which is great of course! But, redoing the > (potentially expensive) computation to prepare the CC for a more > complicated condition is not a good idea. Also, Power's conditional > branch insns just branch on one of the 32 condition bits (either set or > unset), not on a logical combination of multiple of those bits, as we > need with LTGT, UNLT, UNGT, UNEQ, and LE and GE without fastmath. So it > is much cleaner (and causes fewer problems later on) if we only allow > those codes we do support. > > Example of LTGT: > fcmpu 0,0,1 # compare f0 <=> f1 to cr0 (exactly one of > # cr0.lt, cr0.gt, cr0.eq, cr0.un will be set) > cror 2,0,1 # cr0.eq = cr0.lt | cr0.gt > beq 0 # branch if cr0.eq is set > > So, we want the cbranchcc4 here to just do that last insn, not the last > two insns (or all three as any other cbranch<mode>4 is!) Anyhow - my question still stands - what's the fallback for the callers that do not check for failure? How are we sure we're not running into these when relaxing the requirement that a MODE_CC prepare_cmp_insn must not fail? Richard. > > Segher
On Mon, Nov 28, 2022 at 07:46:07PM +0100, Richard Biener wrote: > Anyhow - my question still stands - what's the fallback for the callers > that do not check for failure? How are we sure we're not running into > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > must not fail? This will work the same as with any other define_expand? If the caller of gen_blablabla does not check for failure, you end up with a NULL_RTX in the instruction stream, which will ICE sooner or later. Not pretty, sure, but at least it is a reliable ICE :-) Segher
Hi Richard, 在 2022/11/29 2:46, Richard Biener 写道: > Anyhow - my question still stands - what's the fallback for the callers > that do not check for failure? How are we sure we're not running into > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > must not fail? I examed the code and found that currently callers should be fine with returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called by following callers. 1 gen_cond_trap which doesn't uses MODE_CC 2 prepare_cmp_insn itself where is after MODE_CC processing, so it never hits MODE_CC 3 emit_cmp_and_jump_insns which doesn't uses MODE_CC 4 emit_conditional_move which checks the output is null or not 5 emit_conditional_add which checks the output is null or not Not sure if I missed something. Looking forward to your advice. Thanks a lot Gui Haochen
On Tue, Nov 29, 2022 at 2:15 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > > Hi Richard, > > 在 2022/11/29 2:46, Richard Biener 写道: > > Anyhow - my question still stands - what's the fallback for the callers > > that do not check for failure? How are we sure we're not running into > > these when relaxing the requirement that a MODE_CC prepare_cmp_insn > > must not fail? > > I examed the code and found that currently callers should be fine with > returning a NULL_RTX for MODE_CC processing. The prepare_cmp_insn is called > by following callers. > > 1 gen_cond_trap which doesn't uses MODE_CC > 2 prepare_cmp_insn itself where is after MODE_CC processing, so it never > hits MODE_CC > 3 emit_cmp_and_jump_insns which doesn't uses MODE_CC > 4 emit_conditional_move which checks the output is null or not > 5 emit_conditional_add which checks the output is null or not Thanks for checking. > Not sure if I missed something. Looking forward to your advice. I'd then say the non-presence of the optab should be handled the same as a mismatching predicate as the other comment on the patch indicates. thanks, Richard. > Thanks a lot > Gui Haochen >
diff --git a/gcc/optabs.cc b/gcc/optabs.cc index 165f8d1fa22..3ec8f6b17ba 100644 --- a/gcc/optabs.cc +++ b/gcc/optabs.cc @@ -4484,8 +4484,9 @@ prepare_cmp_insn (rtx x, rtx y, enum rtx_code comparison, rtx size, { enum insn_code icode = optab_handler (cbranch_optab, CCmode); test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y); - gcc_assert (icode != CODE_FOR_nothing - && insn_operand_matches (icode, 0, test)); + gcc_assert (icode != CODE_FOR_nothing); + if (!insn_operand_matches (icode, 0, test)) + goto fail; *ptest = test; return; }