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 16BC03858D1E; Mon, 19 Dec 2022 16:34:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 16BC03858D1E 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 2BJGXt7v023658; Mon, 19 Dec 2022 10:33:56 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2BJGXtnl023657; Mon, 19 Dec 2022 10:33:55 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 19 Dec 2022 10:33:55 -0600 From: Segher Boessenkool To: Jiufu Guo Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com, linkw@gcc.gnu.org Subject: Re: [PATCH V7] rs6000: Optimize cmp on rotated 16bits constant Message-ID: <20221219163355.GO25951@gate.crashing.org> References: <20221219140645.34011-1-guojiufu@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221219140645.34011-1-guojiufu@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,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! Mostlt nitpicking left: On Mon, Dec 19, 2022 at 10:06:45PM +0800, Jiufu Guo wrote: > When checking eq/ne with a constant which has only 16bits, it can be > optimized to check the rotated data. By this, the constant building > is optimized. > > As the example in PR103743: > For "in == 0x8000000000000000LL", this patch generates: > rotldi 3,3,1 ; cmpldi 0,3,1 > instead of: > li 9,-1 ; rldicr 9,9,0,0 ; cmpd 0,3,9 Excellent :-) > * config/rs6000/rs6000-protos.h (can_be_rotated_to_lowbits): New. > (can_be_rotated_to_positive_16bits): New. > (can_be_rotated_to_negative_15bits): New. > * config/rs6000/rs6000.cc (can_be_rotated_to_lowbits): New definition. > (can_be_rotated_to_positive_16bits): New definition. > (can_be_rotated_to_negative_15bits): New definition. > * config/rs6000/rs6000.md (*rotate_on_cmpdi): New define_insn_and_split. Good names. Great function comments as well. > +/* Check if C (as 64bit integer) can be rotated to a constant which constains > + nonzero bits at LOWBITS only. "at the LOWBITS low bits only". Well it probably is clear what is meant :-) > + Return true if C can be rotated to such constant. And *ROT is written to > + the number by which C is rotated. > + Return false otherwise. */ "If so, *ROT is written" etc. > +(define_code_iterator eqne [eq ne]) You should say in the changelog that "eqne" was moved. "(eqne): Move earlier." is plenty of course. > +(define_insn_and_split "*rotate_on_cmpdi" > + rtx note = find_reg_note (curr_insn, REG_BR_PROB, 0); Move this much later please, to just before it is used. > + /* keep the probability info for the prediction of the branch insn. */ "Keep", sentences start with a capital. > +} > +) These go on one line, as just }) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr103743.c > @@ -0,0 +1,52 @@ > +/* { dg-options "-O2" } */ > +/* { dg-do compile { target has_arch_ppc64 } } */ > + > +/* { dg-final { scan-assembler-times {\mcmpldi\M} 10 } } */ > +/* { dg-final { scan-assembler-times {\mcmpdi\M} 4 } } */ > +/* { dg-final { scan-assembler-times {\mrotldi\M} 14 } } */ > + With so much going on in just one function, I am a bit worried that this testcase will easily fail in the future. We will see. Okay for trunk with those i's dotted. Thank you! Segher