Hi Segher,
Thanks for your review, and helpful comments!
Segher Boessenkool <seg...@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
