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

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

Hi Segher,

Thanks for your review, and helpful comments!

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

> 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 :-)
Update.  Thanks :)
>
>> +   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.
Great.  Updated.
>
>> +(define_code_iterator eqne [eq ne])
>
> You should say in the changelog that "eqne" was moved.
> "(eqne): Move earlier." is plenty of course.
Sure, thanks! Update.
>
>> +(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.
Oh, thanks! Update.
>
>> +  /* keep the probability info for the prediction of the branch insn.  */
>
> "Keep", sentences start with a capital.
Update.
>
>> +}
>> +)
>
> These go on one line, as just
> })
Updated.
>
>> --- /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.
Thanks for pointing out this. I understand your concern!
I would pay attention to this.

>
> Okay for trunk with those i's dotted.  Thank you!
Updated and committed via r13-4803-g1060cd2ad00b51.


BR,
Jeff (Jiufu)
>
>
> Segher