public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]
@ 2022-09-07  7:08 Jiufu Guo
  2022-09-23  2:39 ` Jiufu Guo
  2022-09-26  6:21 ` Kewen.Lin
  0 siblings, 2 replies; 4+ messages in thread
From: Jiufu Guo @ 2022-09-07  7:08 UTC (permalink / raw)
  To: gcc-patches; +Cc: segher, dje.gcc, linkw, guojiufu

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]
  2022-09-07  7:08 [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460] Jiufu Guo
@ 2022-09-23  2:39 ` Jiufu Guo
  2022-09-26  6:21 ` Kewen.Lin
  1 sibling, 0 replies; 4+ messages in thread
From: Jiufu Guo @ 2022-09-23  2:39 UTC (permalink / raw)
  To: gcc-patches; +Cc: segher, dje.gcc, linkw

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;
> +}
> +

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]
  2022-09-07  7:08 [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460] Jiufu Guo
  2022-09-23  2:39 ` Jiufu Guo
@ 2022-09-26  6:21 ` Kewen.Lin
  2022-09-27  5:25   ` Jiufu Guo
  1 sibling, 1 reply; 4+ messages in thread
From: Kewen.Lin @ 2022-09-26  6:21 UTC (permalink / raw)
  To: Jiufu Guo; +Cc: dje.gcc, segher, linkw, gcc-patches

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460]
  2022-09-26  6:21 ` Kewen.Lin
@ 2022-09-27  5:25   ` Jiufu Guo
  0 siblings, 0 replies; 4+ messages in thread
From: Jiufu Guo @ 2022-09-27  5:25 UTC (permalink / raw)
  To: Kewen.Lin; +Cc: dje.gcc, segher, linkw, gcc-patches

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-09-27  5:25 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-07  7:08 [PATCH V3] rs6000: cannot_force_const_mem for HIGH code rtx[PR106460] Jiufu Guo
2022-09-23  2:39 ` Jiufu Guo
2022-09-26  6:21 ` Kewen.Lin
2022-09-27  5:25   ` Jiufu Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).