From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id EE7583959CB1 for ; Wed, 16 Nov 2022 12:07:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EE7583959CB1 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 2AGC6K1s028072; Wed, 16 Nov 2022 06:06:20 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2AGC6JNv028070; Wed, 16 Nov 2022 06:06:19 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Wed, 16 Nov 2022 06:06:19 -0600 From: Segher Boessenkool To: David Edelsohn Cc: HAO CHEN GUI , gcc-patches , "Kewen.Lin" , Peter Bergner Subject: Re: [rs6000, patch] Enable have_cbranchcc4 on rs6000 Message-ID: <20221116120619.GU25951@gate.crashing.org> References: <153badc6-8afc-0695-32b2-ab5a9e0a161d@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,KAM_NUMSUBJECT,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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. cbranch4 normally compares the 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, cbranch4 now means "compare and branch", except when 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 . */ 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