diff mbox series

[V3,2/3] Using pli for constant splitting

Message ID 20231206052427.143889-2-guojiufu@linux.ibm.com
State New
Headers show
Series [V3,1/3] rs6000: update num_insns_constant for 2 insns | expand

Commit Message

Jiufu Guo Dec. 6, 2023, 5:24 a.m. UTC 
Hi,

For constant building e.g. r120=0x66666666, which does not fit 'li or lis',
'pli' is used to build this constant via 'emit_move_insn'.

While for a complicated constant, e.g. 0x6666666666666666ULL, when using
'rs6000_emit_set_long_const' to split the constant recursively, it fails to
use 'pli' to build the half part constant: 0x66666666.

'rs6000_emit_set_long_const' could be updated to use 'pli' to build half
part of the constant when necessary.  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-November/636567.html
This verion is refreshed and added with a new testcase.

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/const_split_pli.c: New test.

---
 gcc/config/rs6000/rs6000.cc                        | 7 +++++++
 gcc/testsuite/gcc.target/powerpc/const_split_pli.c | 9 +++++++++
 2 files changed, 16 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/const_split_pli.c

Comments

Kewen.Lin Dec. 7, 2023, 6:12 a.m. UTC | #1 
Hi Jeff,

on 2023/12/6 13:24, Jiufu Guo wrote:
> Hi,
> 
> For constant building e.g. r120=0x66666666, which does not fit 'li or lis',
> 'pli' is used to build this constant via 'emit_move_insn'.
> 
> While for a complicated constant, e.g. 0x6666666666666666ULL, when using
> 'rs6000_emit_set_long_const' to split the constant recursively, it fails to
> use 'pli' to build the half part constant: 0x66666666.
> 
> 'rs6000_emit_set_long_const' could be updated to use 'pli' to build half
> part of the constant when necessary.  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-November/636567.html
> This verion is refreshed and added with a new testcase.
> 
> 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/const_split_pli.c: New test.

Nit: Now we have:

gcc/testsuite/gcc.target/powerpc/const-build.c
gcc/testsuite/gcc.target/powerpc/const_anchors.c
gcc/testsuite/gcc.target/powerpc/const-compare.c

I prefer the name of this new case is like const-build-1.c
(put a detailed comment inside) or const-build-split-pli.c,
to align with the existing.

> 
> ---
>  gcc/config/rs6000/rs6000.cc                        | 7 +++++++
>  gcc/testsuite/gcc.target/powerpc/const_split_pli.c | 9 +++++++++
>  2 files changed, 16 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/const_split_pli.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index dbdc72dce5d..2e074a21a05 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10509,6 +10509,13 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns)
>  				       GEN_INT (0xffffffff)));
>    };
>  
> +  if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c))
> +    {
> +      /* li/lis/pli */
> +      count_or_emit_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/const_split_pli.c b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
> new file mode 100644
> index 00000000000..626c93084aa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile { target lp64 } } */
> +/* { dg-options "-O2" } */

It needs -mdejagnu-cpu=power10 as well.

> +/* { dg-require-effective-target power10_ok } */
> +
> +unsigned long long msk66() { return 0x6666666666666666ULL; }
> +
> +/* { dg-final { scan-assembler-times {\mpli\M} 1 } } */
> +/* { dg-final { scan-assembler-not {\mli\M} } } */
> +/* { dg-final { scan-assembler-not {\mlis\M} } } */

OK for trunk with the above nits tweaked, thanks!

BR,
Kewen
Jiufu Guo Dec. 8, 2023, 3:32 a.m. UTC | #2 
Hi,

Thanks for your insight and helpful review!

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

> Hi Jeff,
>
> on 2023/12/6 13:24, Jiufu Guo wrote:
>> Hi,
>> 
>> For constant building e.g. r120=0x66666666, which does not fit 'li or lis',
>> 'pli' is used to build this constant via 'emit_move_insn'.
>> 
>> While for a complicated constant, e.g. 0x6666666666666666ULL, when using
>> 'rs6000_emit_set_long_const' to split the constant recursively, it fails to
>> use 'pli' to build the half part constant: 0x66666666.
>> 
>> 'rs6000_emit_set_long_const' could be updated to use 'pli' to build half
>> part of the constant when necessary.  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-November/636567.html
>> This verion is refreshed and added with a new testcase.
>> 
>> 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/const_split_pli.c: New test.
>
> Nit: Now we have:
>
> gcc/testsuite/gcc.target/powerpc/const-build.c
> gcc/testsuite/gcc.target/powerpc/const_anchors.c
> gcc/testsuite/gcc.target/powerpc/const-compare.c
>
> I prefer the name of this new case is like const-build-1.c
> (put a detailed comment inside) or const-build-split-pli.c,
> to align with the existing.
Thanks!
>
>> 
>> ---
>>  gcc/config/rs6000/rs6000.cc                        | 7 +++++++
>>  gcc/testsuite/gcc.target/powerpc/const_split_pli.c | 9 +++++++++
>>  2 files changed, 16 insertions(+)
>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/const_split_pli.c
>> 
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index dbdc72dce5d..2e074a21a05 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -10509,6 +10509,13 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns)
>>  				       GEN_INT (0xffffffff)));
>>    };
>>  
>> +  if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c))
>> +    {
>> +      /* li/lis/pli */
>> +      count_or_emit_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/const_split_pli.c b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
>> new file mode 100644
>> index 00000000000..626c93084aa
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
>> @@ -0,0 +1,9 @@
>> +/* { dg-do compile { target lp64 } } */
>> +/* { dg-options "-O2" } */
>
> It needs -mdejagnu-cpu=power10 as well.
Yeap, thanks.
>
>> +/* { dg-require-effective-target power10_ok } */
>> +
>> +unsigned long long msk66() { return 0x6666666666666666ULL; }
>> +
>> +/* { dg-final { scan-assembler-times {\mpli\M} 1 } } */
>> +/* { dg-final { scan-assembler-not {\mli\M} } } */
>> +/* { dg-final { scan-assembler-not {\mlis\M} } } */
>
> OK for trunk with the above nits tweaked, thanks!
>
> BR,
> Kewen
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index dbdc72dce5d..2e074a21a05 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -10509,6 +10509,13 @@  rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c, int *num_insns)
 				       GEN_INT (0xffffffff)));
   };
 
+  if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (c))
+    {
+      /* li/lis/pli */
+      count_or_emit_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/const_split_pli.c b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
new file mode 100644
index 00000000000..626c93084aa
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/const_split_pli.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target power10_ok } */
+
+unsigned long long msk66() { return 0x6666666666666666ULL; }
+
+/* { dg-final { scan-assembler-times {\mpli\M} 1 } } */
+/* { dg-final { scan-assembler-not {\mli\M} } } */
+/* { dg-final { scan-assembler-not {\mlis\M} } } */