diff mbox series

[V2,2/3] Using pli to split 34bits constant

Message ID 20231115030237.1188073-2-guojiufu@linux.ibm.com
State Unresolved
Headers
Series [V2,1/3] rs6000: update num_insns_constant for 2 insns |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jiufu Guo Nov. 15, 2023, 3:02 a.m. UTC 
  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.

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(+)

Comments

Kewen.Lin Nov. 22, 2023, 9:18 a.m. UTC | #1 
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 } } */
Jiufu Guo Nov. 27, 2023, 2:49 a.m. UTC | #2 
Hi,

"Kewen.Lin" <linkw@linux.ibm.com> writes:

> 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.

I would update the message to make it clear.
Thanks so much for your suggestions!

BR,
Jeff (Jiufu Guo)


>
> 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 } } */
diff mbox series

Patch

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 } } */