Re: [PATCH V2 2/3] Using pli to split 34bits constant

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi,

on 2023/11/15 11:02, Jiufu Guo wrote:
> Hi,
> 
> For constants with 16bit values, 'li or lis' can be used to generate
> the value.  For 34bit constant, 'pli' is ok to generate the value.
> For example: 0x6666666666666666ULL, "pli 3,1717986918; rldimi 3,3,32,0"
> can be used.

Since now if emit_move_insn with a 34bit constant, it's already adopting
pli.  So it's not obvious to the readers why we want this change, I think
you should probably state the reason here explicitly, like in function 
rs6000_emit_set_long_const it's possible to recursively call itself without
invoking emit_move_insn, then it can result in sub-optimal constant build ...
And for the testing I prefer to have a dedicated test case for it, like
extracting function msk66 from pr93012.c and checking its generated assembly
has pli but not lis and ori on Power10 and up.

The others look good to me.  Thanks!

BR,
Kewen

> 
> Compare with previous:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634196.html
> This verion updates a testcase to cover this functionality.
> 
> Bootstrap&regtest pass on ppc64{,le}.
> Is this ok for trunk?
> 
> BR,
> Jeff (Jiufu Guo)
> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/rs6000.cc (rs6000_emit_set_long_const): Add code to use
> 	pli for 34bit constant.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/pr93012.c: Update to check pli.
> 
> ---
>  gcc/config/rs6000/rs6000.cc                | 9 +++++++++
>  gcc/testsuite/gcc.target/powerpc/pr93012.c | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index ba40dd6eee4..b277c52687b 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10504,6 +10504,15 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns)
>        return;                                                                  \
>      }
> 
> +  if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c))
> +    {
> +      /* li/lis/pli */
> +      ADJUST_INSN_NUM_AND_RET (1);
> +
> +      emit_move_insn (dest, GEN_INT (c));
> +      return;
> +    }
> +
>    if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (ud1 & 0x8000))
>        || (ud4 == 0 && ud3 == 0 && ud2 == 0 && !(ud1 & 0x8000)))
>      {
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr93012.c b/gcc/testsuite/gcc.target/powerpc/pr93012.c
> index 4f764d0576f..a07ff764bbf 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr93012.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr93012.c
> @@ -10,4 +10,5 @@ unsigned long long mskh1() { return 0xffff9234ffff9234ULL; }
>  unsigned long long mskl1() { return 0x2bcdffff2bcdffffULL; }
>  unsigned long long mskse() { return 0xffff1234ffff1234ULL; }
> 
> +/* { dg-final { scan-assembler-times {\mpli\M} 4 { target has_arch_pwr10 }} } */
>  /* { dg-final { scan-assembler-times {\mrldimi\M} 7 } } */