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 EF5CF38582BE; Fri, 16 Dec 2022 20:37:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF5CF38582BE 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 2BGKaasY029327; Fri, 16 Dec 2022 14:36:36 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2BGKaZpm029326; Fri, 16 Dec 2022 14:36:35 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 16 Dec 2022 14:36:35 -0600 From: Segher Boessenkool To: Jiufu Guo Cc: gcc-patches@gcc.gnu.org, dje.gcc@gmail.com, linkw@gcc.gnu.org Subject: Re: [PATCH V6] rs6000: Optimize cmp on rotated 16bits constant Message-ID: <20221216203635.GF25951@gate.crashing.org> References: <20220829034216.94029-1-guojiufu@linux.ibm.com> <20221213210232.GM25951@gate.crashing.org> <7e8rjajsq9.fsf@pike.rch.stglabs.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7e8rjajsq9.fsf@pike.rch.stglabs.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,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: On Wed, Dec 14, 2022 at 04:26:54PM +0800, Jiufu Guo wrote: > Segher Boessenkool writes: > > On Mon, Aug 29, 2022 at 11:42:16AM +0800, Jiufu Guo wrote: > >> li %r9,-1 > >> rldicr %r9,%r9,0,0 > >> cmpd %cr0,%r3,%r9 > > > > FWIW, I find the winnt assembler syntax very hard to read, and I doubt > > I am the only one. > Oh, sorry about that. I will avoid to add '-mregnames' to dump asm. :) > BTW, what options are you used to dump asm code? The same as GCC outputs, and as I write assembler code as: bare numbers. It is much easier to type, and very much easier to read. -mregnames is fine for output (and it is the default as well), but problematic for input. Take for example li r10,r10 which translates to li 10,10 while what was probably wanted is to load the address of the global symbol r10, which can be written as li r10,(r10) I do understand that liking the bare numbers syntax is an acquired taste of course. But less clutter is very useful. This goes hand in hand with writing multiple asm statements per line, which allows you to group things together nicely: li 9,-1 ; rldicr 9,9,0,0 ; cmpd 3,9 > > Maybe it is better to not return magic values anyway? So perhaps > > > > bool > > can_be_done_as_compare_of_rotate (unsigned HOST_WIDE_INT c, int clz, int *rot) > > > > (with *rot written if the return value is true). > Thanks for your suggestion! > It is checking if a constant can be rotated from/to a value which has > only few tailing nonzero bits (all leading bits are zeros). > > So, I'm thinking to name the function as something like: > can_be_rotated_to_lowbits. That is a good name yeah. > >> +bool > >> +compare_rotate_immediate_p (unsigned HOST_WIDE_INT c) > > > > No _p please, this function is not a predicate (at least, the name does > > not say what it tests). So a better name please. This matters even > > more for extern functions (like this one) because the function > > implementation is always farther away so you do not easily have all > > interface details in mind. Good names help :-) > Thanks! Name is always a matter. :) > > Maybe we can name this funciton as "can_be_rotated_as_compare_operand", > or "is_constant_rotateable_for_compare", because this function checks > "if a constant can be rotated to/from an immediate operand of > cmpdi/cmpldi". Maybe just "constant_can_be_rotated_to_lowbits"? (If that is what the function does). It doesn't clearly say that it allows negative numbers as well, but that is a problem of the function itself already; maybe it would be better to do signed and unsigned separately. > >> +(define_code_iterator eqne [eq ne]) > >> +(define_code_attr EQNE [(eq "EQ") (ne "NE")]) > > > > Just or should work? > Great! Thanks for point out this! works. > > > > Please fix these things. Almost there :-) > > I updated the patch as below. Bootstraping and regtesting is ongoing. > Thanks again for your careful and insight review! Please send as new message (not as reply even), that is much easier to handle. Thanks! Segher