Re: [PATCH V6] rs6000: Optimize cmp on rotated 16bits constant

[Jiufu Guo <guojiufu@linux.ibm.com>]

Hi,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Wed, Dec 14, 2022 at 04:26:54PM +0800, Jiufu Guo wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> 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
>
Great!  Thanks for your helpful comments!
>> > 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.
It makes sense. I updated a new version patch.
>
>> >> +(define_code_iterator eqne [eq ne])
>> >> +(define_code_attr EQNE [(eq "EQ") (ne "NE")])
>> >
>> > Just <CODE> or <CODE:eqne> should work?
>> Great! Thanks for point out this! <eqne:CODE> 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!

Sure, I just submit a new patch version.
https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608765.html

Thanks a lot for your review.


BR,
Jeff (Jiufu)

>
>
> Segher