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

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

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