[PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]

[PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]

[Jiufu Guo]

Hi,

As the issue in PR106460, a rtx 'high:DI (symbol_ref:DI ("var_48")' is tried
to store into constant pool and ICE occur.  But actually, this rtx represents
partial address and can not be put into a .rodata section.

This patch updates rs6000_cannot_force_const_mem to return true for rtx(s) with
HIGH code, because these rtx(s) indicate part of address and are not ok for
constant pool.

Below are some examples:
(high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
(high:DI (symbol_ref:DI ("var_1")..)))

This patch updated the previous patch, and drafted an test case which ICE
without the patch, and assoicated with one PR.
https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html
This patch also updated the message for previous patch V2.

I would ask help to review this patch one more time.

Bootstrap and regtest pass on ppc64 and ppc64le.
Is this ok for trunk.

BR,
Jeff(Jiufu)

	PR target/106460

gcc/ChangeLog:

	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true
	for HIGH code rtx.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr106460.c: New test.
---
 gcc/config/rs6000/rs6000.cc                 |  7 +++++--
 gcc/testsuite/gcc.target/powerpc/pr106460.c | 11 +++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106460.c

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 2f3146e56f8..04e3a393147 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -9643,8 +9643,11 @@ rs6000_init_stack_protect_guard (void)
 static bool
 rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
 {
-  if (GET_CODE (x) == HIGH
-      && GET_CODE (XEXP (x, 0)) == UNSPEC)
+  /* If GET_CODE (x) is HIGH, the 'X' represets the high part of a symbol_ref.
+     It indicates partial address,  which can not be put into a constant pool.
+     e.g.  (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
+     (high:DI (symbol_ref:DI ("var")..)).  */
+  if (GET_CODE (x) == HIGH)
     return true;
 
   /* A TLS symbol in the TOC cannot contain a sum.  */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr106460.c b/gcc/testsuite/gcc.target/powerpc/pr106460.c
new file mode 100644
index 00000000000..dfaffcb6e28
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr106460.c
@@ -0,0 +1,11 @@
+/* { dg-options "-O1 -mdejagnu-cpu=power10" } */
+
+/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */
+extern short var_48;
+void
+foo (double *r)
+{
+  if (var_48)
+    *r = 1234.5678;
+}
+
-- 
2.17.1

Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]

[Jiufu Guo]

Hi,

Gentle ping:
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601190.html

BR,
Jeff (Jiufu)

Jiufu Guo <guojiufu@linux.ibm.com> writes:

> Hi,
>
> As the issue in PR106460, a rtx 'high:DI (symbol_ref:DI ("var_48")' is tried
> to store into constant pool and ICE occur.  But actually, this rtx represents
> partial address and can not be put into a .rodata section.
>
> This patch updates rs6000_cannot_force_const_mem to return true for rtx(s) with
> HIGH code, because these rtx(s) indicate part of address and are not ok for
> constant pool.
>
> Below are some examples:
> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
> (high:DI (symbol_ref:DI ("var_1")..)))
>
> This patch updated the previous patch, and drafted an test case which ICE
> without the patch, and assoicated with one PR.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html
> This patch also updated the message for previous patch V2.
>
> I would ask help to review this patch one more time.
>
> Bootstrap and regtest pass on ppc64 and ppc64le.
> Is this ok for trunk.
>
> BR,
> Jeff(Jiufu)
>
> 	PR target/106460
>
> gcc/ChangeLog:
>
> 	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true
> 	for HIGH code rtx.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/powerpc/pr106460.c: New test.
> ---
>  gcc/config/rs6000/rs6000.cc                 |  7 +++++--
>  gcc/testsuite/gcc.target/powerpc/pr106460.c | 11 +++++++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106460.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2f3146e56f8..04e3a393147 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -9643,8 +9643,11 @@ rs6000_init_stack_protect_guard (void)
>  static bool
>  rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>  {
> -  if (GET_CODE (x) == HIGH
> -      && GET_CODE (XEXP (x, 0)) == UNSPEC)
> +  /* If GET_CODE (x) is HIGH, the 'X' represets the high part of a symbol_ref.
> +     It indicates partial address,  which can not be put into a constant pool.
> +     e.g.  (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
> +     (high:DI (symbol_ref:DI ("var")..)).  */
> +  if (GET_CODE (x) == HIGH)
>      return true;
>  
>    /* A TLS symbol in the TOC cannot contain a sum.  */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106460.c b/gcc/testsuite/gcc.target/powerpc/pr106460.c
> new file mode 100644
> index 00000000000..dfaffcb6e28
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106460.c
> @@ -0,0 +1,11 @@
> +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */
> +
> +/* (high:DI (symbol_ref:DI ("var_48")..))) should not cause ICE. */
> +extern short var_48;
> +void
> +foo (double *r)
> +{
> +  if (var_48)
> +    *r = 1234.5678;
> +}
> +

Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]

[Kewen.Lin]

Hi Jeff,

on 2022/9/7 15:08, Jiufu Guo via Gcc-patches wrote:
> Hi,
> 
> As the issue in PR106460, a rtx 'high:DI (symbol_ref:DI ("var_48")' is tried
> to store into constant pool and ICE occur.  But actually, this rtx represents
> partial address and can not be put into a .rodata section.
> 
> This patch updates rs6000_cannot_force_const_mem to return true for rtx(s) with
> HIGH code, because these rtx(s) indicate part of address and are not ok for
> constant pool.
> 
> Below are some examples:
> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
> (high:DI (symbol_ref:DI ("var_1")..)))
> 
> This patch updated the previous patch, and drafted an test case which ICE
> without the patch, and assoicated with one PR.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html
> This patch also updated the message for previous patch V2.
> 
> I would ask help to review this patch one more time.
> 
> Bootstrap and regtest pass on ppc64 and ppc64le.
> Is this ok for trunk.
> 
> BR,
> Jeff(Jiufu)
> 
> 	PR target/106460
> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true
> 	for HIGH code rtx.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/pr106460.c: New test.
> ---
>  gcc/config/rs6000/rs6000.cc                 |  7 +++++--
>  gcc/testsuite/gcc.target/powerpc/pr106460.c | 11 +++++++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106460.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2f3146e56f8..04e3a393147 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -9643,8 +9643,11 @@ rs6000_init_stack_protect_guard (void)
>  static bool
>  rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>  {
> -  if (GET_CODE (x) == HIGH
> -      && GET_CODE (XEXP (x, 0)) == UNSPEC)
> +  /* If GET_CODE (x) is HIGH, the 'X' represets the high part of a symbol_ref.
> +     It indicates partial address,  which can not be put into a constant pool.
> +     e.g.  (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
> +     (high:DI (symbol_ref:DI ("var")..)).  */

Nit: Maybe it's good to align these two "(high:DI ... ?

> +  if (GET_CODE (x) == HIGH)
>      return true;
>  
>    /* A TLS symbol in the TOC cannot contain a sum.  */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106460.c b/gcc/testsuite/gcc.target/powerpc/pr106460.c
> new file mode 100644
> index 00000000000..dfaffcb6e28
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106460.c
> @@ -0,0 +1,11 @@

Need a power10_ok effective target here.

/* { dg-require-effective-target power10_ok } */

> +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */

Nit: As Segher's review on one of my patches, O2 is preferred against O1 if it
still works for this issue.  The point is to avoid some related optimization
(routines or passes) to be disabled at O1 one day and this becomes ineffective.

BR,
Kewen

Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]

[Jiufu Guo]

Hi,

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

> Hi Jeff,
>
> on 2022/9/7 15:08, Jiufu Guo via Gcc-patches wrote:
>> Hi,
>> 
>> As the issue in PR106460, a rtx 'high:DI (symbol_ref:DI ("var_48")' is tried
>> to store into constant pool and ICE occur.  But actually, this rtx represents
>> partial address and can not be put into a .rodata section.
>> 
>> This patch updates rs6000_cannot_force_const_mem to return true for rtx(s) with
>> HIGH code, because these rtx(s) indicate part of address and are not ok for
>> constant pool.
>> 
>> Below are some examples:
>> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
>> (high:DI (symbol_ref:DI ("var_1")..)))
>> 
>> This patch updated the previous patch, and drafted an test case which ICE
>> without the patch, and assoicated with one PR.
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-July/597712.html
>> This patch also updated the message for previous patch V2.
>> 
>> I would ask help to review this patch one more time.
>> 
>> Bootstrap and regtest pass on ppc64 and ppc64le.
>> Is this ok for trunk.
>> 
>> BR,
>> Jeff(Jiufu)
>> 
>> 	PR target/106460
>> 
>> gcc/ChangeLog:
>> 
>> 	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem): Return true
>> 	for HIGH code rtx.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	* gcc.target/powerpc/pr106460.c: New test.
>> ---
>>  gcc/config/rs6000/rs6000.cc                 |  7 +++++--
>>  gcc/testsuite/gcc.target/powerpc/pr106460.c | 11 +++++++++++
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106460.c
>> 
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index 2f3146e56f8..04e3a393147 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -9643,8 +9643,11 @@ rs6000_init_stack_protect_guard (void)
>>  static bool
>>  rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>>  {
>> -  if (GET_CODE (x) == HIGH
>> -      && GET_CODE (XEXP (x, 0)) == UNSPEC)
>> +  /* If GET_CODE (x) is HIGH, the 'X' represets the high part of a symbol_ref.
>> +     It indicates partial address,  which can not be put into a constant pool.
>> +     e.g.  (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
>> +     (high:DI (symbol_ref:DI ("var")..)).  */
>
> Nit: Maybe it's good to align these two "(high:DI ... ?
OK, thanks! 
>
>> +  if (GET_CODE (x) == HIGH)
>>      return true;
>>  
>>    /* A TLS symbol in the TOC cannot contain a sum.  */
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106460.c b/gcc/testsuite/gcc.target/powerpc/pr106460.c
>> new file mode 100644
>> index 00000000000..dfaffcb6e28
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr106460.c
>> @@ -0,0 +1,11 @@
>
> Need a power10_ok effective target here.
>
> /* { dg-require-effective-target power10_ok } */
OK, will add this.
>
>> +/* { dg-options "-O1 -mdejagnu-cpu=power10" } */
>
> Nit: As Segher's review on one of my patches, O2 is preferred against O1 if it
> still works for this issue.  The point is to avoid some related optimization
> (routines or passes) to be disabled at O1 one day and this becomes ineffective.
>
Yeap.  While, for this case, the ICE is not reproduciable with -O2.
So, -O1 is used here.

BR,
Jeff (Jiufu)

> BR,
> Kewen