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 C75E23858D1E; Tue, 23 Aug 2022 22:19:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C75E23858D1E 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 27NMItTU018913; Tue, 23 Aug 2022 17:18:55 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 27NMItAv018910; Tue, 23 Aug 2022 17:18:55 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 23 Aug 2022 17:18:55 -0500 From: Segher Boessenkool To: Jiufu Guo Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com, linkw@gcc.gnu.org Subject: Re: [PATCH V4] rs6000: Optimize cmp on rotated 16bits constant Message-ID: <20220823221854.GX25951@gate.crashing.org> References: <20220725132922.45470-1-guojiufu@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220725132922.45470-1-guojiufu@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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! On Mon, Jul 25, 2022 at 09:29:22PM +0800, Jiufu Guo wrote: > When checking eq/neq with a constant which has only 16bits, it can be > optimized to check the rotated data. By this, the constant building > is optimized. "ne", not "neq". > gcc/ChangeLog: > > * config/rs6000/rs6000-protos.h (rotate_from_leading_zeros_const): > New decl. "New declaration." or just "New". Also, don't break lines early please, especially if that means ending a line in a colon, which then looks as if you forgot to write something there. > * config/rs6000/rs6000.cc (rotate_from_leading_zeros_const): New > define for checking simply rotated constant. "New." or "New definition." or such. > +/* Check if C can be rotated from an immediate which contains leading > + zeros at least CLZ. "Which starts (as 64 bit integer) with at least CLZ bits zero" or such. > + /* xx0..0xx: rotate enough bits firstly, then check case a. */ > + const int rot_bits = HOST_BITS_PER_WIDE_INT - clz + 1; > + unsigned HOST_WIDE_INT rc = (c >> rot_bits) | (c << (clz - 1)); > + tz = ctz_hwi (rc); > + if (clz_hwi (rc) + tz >= clz) > + return tz + rot_bits; This could use some more explanation. > +(define_code_iterator eqne [eq ne]) > +;; "i == C" ==> "rotl(i,N) == rotl(C,N)" > +(define_insn_and_split "*rotate_on_cmpdi" > + [(set (pc) > + (if_then_else (eqne (match_operand:DI 1 "gpc_reg_operand" "r") Wrong indentation. The ( should be in the same column as the preceding ( it matches. Setting "pc" to either 0 or 1 is never correct. > + "TARGET_POWERPC64 && !reload_completed && can_create_pseudo_p () reload_completed in splitters is almost always wrong. It isn't any better if it is in the insn condition of a define_insn_and_split :-) > + && num_insns_constant (operands[2], DImode) > 1 > + && (rotate_from_leading_zeros_const (~UINTVAL (operands[2]), 49) > 0 > + || rotate_from_leading_zeros_const (UINTVAL (operands[2]), 48) > 0)" There must be a better way to describe this. > + if (rot < 0) > + { > + sgn = true; > + rot = rotate_from_leading_zeros_const (~C, 49); > + } Bad indentation. > + rtx cmp = ne ? gen_rtx_NE (CCmode, cc, const0_rtx) > + : gen_rtx_EQ (CCmode, cc, const0_rtx); rtx cmp = gen_rtx_ (...); (and define a code_attr EQNE to just output the uppercase EQ or NE). Why is this doing a conditional branch at all? Unpredictable conditional branches are extremely costly. > +/* { dg-require-effective-target lp64 } */ arch_ppc64 > +/* { dg-final { scan-assembler-times "cmpldi" 10 } } */ > +/* { dg-final { scan-assembler-times "cmpdi" 4 } } */ > +/* { dg-final { scan-assembler-times "rotldi" 14 } } */ Please use \m and \M . Thanks, Segher