Re: [PATCH 1/2] Using pli(paddi) and rotate to build 64bit constants

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

Hi,

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

> Hi!
>
> On Fri, Sep 02, 2022 at 02:56:21PM +0800, Jiufu Guo wrote:
>> >> +      /* pli 9,high32 + sldi 9,32 + paddi 9,9,low32.  */
>> >> +      else
>> >> +	{
>> >
>> > The comment goes here, in the block it refers to.  Comments for a block
>> > are the first thing *in* the block.
>> OK, great! I like the format you sugguested here :-)
>
> It's the normal GCC style, not my invention :-)
>
>> >> +	  emit_move_insn (copy_rtx (dest), GEN_INT ((ud4 << 16) | ud3));
>> >> +
>> >> +	  emit_move_insn (copy_rtx (dest),
>> >> +			  gen_rtx_ASHIFT (DImode, copy_rtx (dest),
>> >> +					  GEN_INT (32)));
>> >> +
>> >> +	  bool can_use_paddi = REGNO (dest) != FIRST_GPR_REGNO;
>> >
>> > There should be a test that we so the right thing (or *a* right thing,
>> > anyway; a working thing; but hopefully a reasonably fast thing) for
>> > !can_use_paddi.
>> To catch this test point, we need let the splitter run after RA,
>> and register 0 happen to be the dest of an assignment.
>
> Or force the testcase to use r0 some other way.  Well, "forcing" cannot
> be done, but we can probably encourage it (via a local register asm for
> example, or by tying the var to the output of an asm that is hard reg 0,
> or perhaps there are other ways as well :-) )
Specify register is very helpful here! Both below two cases could help
register 0 to be selected for a variable.  Great!  Thanks!
code1
-------
void __attribute__ ((noinline)) foo (unsigned long long *a)
{
  register long long d asm("r0") = 0x1245abcef9240dec;
  long long n;
  asm("cntlzd %0, %1" : "=r"(n) : "r"(d));
  *a = n;
}

code2
-----------
register long long d asm ("r0");

void __attribute__ ((noinline)) foo ()
{
  d = 0x2351847027482577ULL;
}


>
>> I will add this test case in patch.
>> Is this ok?  Any sugguestions?
>
> Sounds useful yes.  Maybe describe the expected output in words as well
> (in the testcase, not in email)?
OK.   Will update the patch accordingly.
>
>> >> +/* 3 insns for each constant: pli+sldi+paddi or pli+pli+rldimi.
>> >> +   And 3 additional insns: std+std+blr: 9 insns totally.  */
>> >> +/* { dg-final { scan-assembler-times {(?n)^\s+[a-z]} 9 } } */
>> >
>> > Also test the expected insns separately please?  The std's (just with
>> > \mstd so it will catch all variations as well), the blr, the pli's and
>> > the rldimi etc.?
>> The reason of using "(?n)^\s+[a-z]" is to keep this test case pass no
>> matter the splitter running before or after RA.
>
> Ah.  Some short comment in the testcase please?
Thanks.  We can check individual instructions to check better asm
pli+pli+rldimi is generated, since the splitter could run in split1 pass.

BR,
Jeff(Jiufu)

>
> Thanks again,
>
>
> Segher