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

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

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