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

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

Hi!

Sorry for the tardiness.

On Mon, Aug 29, 2022 at 11:42:16AM +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 %r3,%r3,16
>         cmpldi %cr0,%r3,32768
> instead:
>         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.

So you're doing
  rotldi 3,3,16 ; cmpldi 3,0x8000
instead of
  li 9,-1 ; rldicr 9,9,0,0 ; cmpd 3,9

> +/* Check if C can be rotated from an immediate which starts (as 64bit integer)
> +   with at least CLZ bits zero.
> +
> +   Return the number by which C can be rotated from the immediate.
> +   Return -1 if C can not be rotated as from.  */
> +
> +int
> +rotate_from_leading_zeros_const (unsigned HOST_WIDE_INT c, int clz)

The name does not say what the function does.  Can you think of a better
name?

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).

> +  /* case c. xx10.....0xx: rotate 'clz + 1' bits firstly, then check case b.

s/firstly/first/

> +/* Check if C can be rotated from an immediate operand of cmpdi or cmpldi.  */
> +
> +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 :-)

> +(define_code_iterator eqne [eq ne])
> +(define_code_attr EQNE [(eq "EQ") (ne "NE")])

Just <CODE> or <CODE:eqne> should work?

Please fix these things.  Almost there :-)


Segher