Re: [PATCH] rs6000: Change GPR2 to volatile & non-fixed register for function that does not use TOC [PR110320]

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

Hi Jeevitha,

on 2023/6/24 00:49, P Jeevitha wrote:
> Hi All,
> 
> The following patch has been bootstrapped and regtested on powerpc64le-linux.
> 
> Normally, GPR2 is the TOC pointer and is defined as a fixed and non-volatile
> register. However, it can be used as volatile for PCREL addressing. Therefore,
> if the code is PCREL and the user is not explicitly requesting TOC addressing,
> then the register r2 can be changed to volatile and non-fixed register. Changes
> in register preservation roles can be accomplished with the help of available
> target hooks (TARGET_CONDITIONAL_REGISTER_USAGE).

Nice, thanks for improving this.

> 
> 2023-06-23  Jeevitha Palanisamy  <jeevitha@linux.ibm.com>
> 
> gcc/
> 	PR target/PR110320
> 	* config/rs6000/rs6000.cc (rs6000_conditional_register_usage): Change
> 	GPR2 to volatile and non-fixed register for pc-relative code.
> 
> gcc/testsuite/
> 	PR target/PR110320
> 	* gcc.target/powerpc/pr110320_1.c: New testcase.
> 	* gcc.target/powerpc/pr110320_2.c: New testcase.
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 546c353029b..9e978f85f9d 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10169,6 +10169,35 @@ rs6000_conditional_register_usage (void)
>    if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
>      call_used_regs[2] = 0;
>  
> +  /* The TOC register is not needed for functions using the PC-relative ABI
> +     extension, so make it available for register allocation as a volatile
> +     register.  */
> +  if (FIXED_R2 && rs6000_pcrel_p ())
> +    {
> +      bool cli_fixedr2 = false;
> +
> +      /* Verify the user has not explicitly asked for GPR2 to be fixed.  */
> +      if (common_deferred_options)
> +	{
> +	  unsigned int idx;
> +	  cl_deferred_option *opt;
> +	  vec<cl_deferred_option> v;
> +	  v = *((vec<cl_deferred_option> *) common_deferred_options);
> +	  FOR_EACH_VEC_ELT (v, idx, opt)
> +	    if (opt->opt_index == OPT_ffixed_ && strcmp (opt->arg,"r2") == 0)
> +	      {
> +		cli_fixedr2 = true;
> +		break;
> +	      }
> +	}

I think the reason why we need to check common_deferred_options is at this time
we can't distinguish the fixed_regs[2] is from the initialization or command line
user explicit specification.  But could we just update the FIXED_REGISTERS without
FIXED_R2 and set FIXED_R2 when it's needed in this function instead?  Then I'd
expect that when we find fixed_regs[2] is set at the beginning of this function, it
would mean users specify it explicitly and then we don't need this option checking?

Besides, IMHO we need a corresponding test case to cover this -ffixed-r2 handling.

> +
> +      /* If GPR2 is not FIXED (eg, not a TOC register), then it is volatile.  */
> +      if (!cli_fixedr2)
> +	{
> +	  fixed_regs[2] = 0;
> +	  call_used_regs[2] = 1;
> +	}
> +    }
>    if (DEFAULT_ABI == ABI_V4 && flag_pic == 2)
>      fixed_regs[RS6000_PIC_OFFSET_TABLE_REGNUM] = 1;
>  
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr110320_1.c b/gcc/testsuite/gcc.target/powerpc/pr110320_1.c
> new file mode 100644
> index 00000000000..42143fbf889
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr110320_1.c
> @@ -0,0 +1,23 @@
> +/* PR target/110320 */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_pcrel } */

Do we have some environment combination which supports powerpc_pcrel but not
power10_ok?  I'd expect that only powerpc_pcrel is enough.

> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -ffixed-r0 -ffixed-r11 -ffixed-r12" } */
> +
> +/* Ensure we use r2 as a normal volatile register for the code below.
> +   The test case ensures all of the parameter registers r3 - r10 are used
> +   and needed after we compute the expression "x + y" which requires a
> +   temporary.  The -ffixed-r* options disallow using the other volatile
> +   registers r0, r11 and r12.  That leaves RA to choose from r2 and the more
> +   expensive non-volatile registers for the temporary to be assigned to, and
> +   RA will always chooses the cheaper volatile r2 register.  */
> +
> +extern long bar (long, long, long, long, long, long, long, long *);
> +
> +long
> +foo (long r3, long r4, long r5, long r6, long r7, long r8, long r9, long *r10)
> +{
> +  *r10 = r3 + r4;
> +  return bar (r3, r4, r5, r6, r7, r8, r9, r10);
> +}
> +
> +/* { dg-final { scan-assembler {\madd 2,3,4\M} } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr110320_2.c b/gcc/testsuite/gcc.target/powerpc/pr110320_2.c
> new file mode 100644
> index 00000000000..9d0da5b9695
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr110320_2.c
> @@ -0,0 +1,22 @@
> +/* PR target/110320 */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_pcrel } */

Ditto.

The others look good to me, thanks!

BR,
Kewen