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

[Segher Boessenkool <segher@kernel.crashing.org>]

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

> > 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 <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!


Segher