allow target config to state r18 is fixed on aarch64

Message ID 3FA70C04-A7B3-4C74-AFF7-7BAC7C2C0DBC@adacore.com
State New
Headers show
Series
  • allow target config to state r18 is fixed on aarch64
Related show

Commit Message

Olivier Hainque Oct. 10, 2018, 9:40 p.m.
Hello,

The aarch64 "platform register" r18 is currently
unconditionally used as a scratch register by gcc.

Working on a VxWorks port for this arch (that we
plan to contribute soon), we discovered that VxWorks
has an internal use of this register so it needs
to be considered "fixed" for this port.

Hence the attached patch proposal, which we have been
using successfully for a while now. It just introduces
a FIXED_R18 config macro, defaulted to 0, which an OS
specific config file may redefine.

Is this ok to commit ?

Thanks in advance,

With Kind Regards,

Olivier


2018-03-18  Olivier Hainque  <hainque@adacore.com>

	* config/aarch64/aarch64.h (FIXED_R18): New
	internal configuration macro, defaulted to 0.
	(FIXED_REGISTERS): Use it.
	(STATIC_CHAIN_REGNUM): Fallback to r11 instead
	of r18 if the latter is fixed.

Comments

Kyrill Tkachov Oct. 12, 2018, 9:50 a.m. | #1
Hi Olivier,

On 10/10/18 22:40, Olivier Hainque wrote:
> Hello,
>
> The aarch64 "platform register" r18 is currently
> unconditionally used as a scratch register by gcc.
>
> Working on a VxWorks port for this arch (that we
> plan to contribute soon), we discovered that VxWorks
> has an internal use of this register so it needs
> to be considered "fixed" for this port.
>
> Hence the attached patch proposal, which we have been
> using successfully for a while now. It just introduces
> a FIXED_R18 config macro, defaulted to 0, which an OS
> specific config file may redefine.
>
> Is this ok to commit ?
>

CC'ing the aarch64 maintainers as they'll have to approve it.
I'm guessing you've tested this in the usual way (bootstrap and test)?

> Thanks in advance,
>
> With Kind Regards,
>
> Olivier
>
>
> 2018-03-18  Olivier Hainque  <hainque@adacore.com>
>
>         * config/aarch64/aarch64.h (FIXED_R18): New
>         internal configuration macro, defaulted to 0.
>         (FIXED_REGISTERS): Use it.
>         (STATIC_CHAIN_REGNUM): Fallback to r11 instead
>         of r18 if the latter is fixed.
>

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index fa9af26..2e69a4d 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -284,11 +284,13 @@ extern unsigned aarch64_architecture_version;
  
  /* Standard register usage.  */
  
+#define FIXED_R18 0
+


Any reason not to use the TARGET_CONDITIONAL_REGISTER_USAGE interface to adjust the fixed_regs table instead?
I'm not objecting to your approach here, just interested if you considered and rejected it.

Thanks,
Kyrill



  /* 31 64-bit general purpose registers R0-R30:
     R30		LR (link register)
     R29		FP (frame pointer)
     R19-R28	Callee-saved registers
-   R18		The platform register; use as temporary register.
+   R18		The platform register; use as temporary register if !FIXED_R18.
     R17		IP1 The second intra-procedure-call temporary register
  		(can be used by call veneers and PLT code); otherwise use
  		as a temporary register
@@ -324,7 +326,7 @@ extern unsigned aarch64_architecture_version;
    {							\
      0, 0, 0, 0,   0, 0, 0, 0,	/* R0 - R7 */		\
      0, 0, 0, 0,   0, 0, 0, 0,	/* R8 - R15 */		\
-    0, 0, 0, 0,   0, 0, 0, 0,	/* R16 - R23 */		\
+    0, 0, FIXED_R18, 0, 0, 0, 0, 0,	/* R16 - R23 */		\
      0, 0, 0, 0,   0, 1, 0, 1,	/* R24 - R30, SP */	\
      0, 0, 0, 0,   0, 0, 0, 0,   /* V0 - V7 */           \
      0, 0, 0, 0,   0, 0, 0, 0,   /* V8 - V15 */		\
@@ -419,7 +421,7 @@ extern unsigned aarch64_architecture_version;
     uses alloca.  */
  #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
  
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+#define STATIC_CHAIN_REGNUM		(!(FIXED_R18) ? R18_REGNUM : R11_REGNUM)
  #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
  #define FRAME_POINTER_REGNUM		SFP_REGNUM
  #define STACK_POINTER_REGNUM		SP_REGNUM
Olivier Hainque Oct. 12, 2018, 6:43 p.m. | #2
Hi Kyrill,

Thanks for your feedback!

> On 12 Oct 2018, at 05:50, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> CC'ing the aarch64 maintainers as they'll have to approve it.
> I'm guessing you've tested this in the usual way (bootstrap and test)?

Sorry, I failed to mention the testing indeed. We don't
have a native box at hand, so I'm not really able to
conduct regular bootstrap and dg testing per se.

We have an active gcc-7 aarch64-vxworks port, still, and have
had good ACATS and internal testsuite results for this port
with this patch, so it doesn't break the build and has
the intended effect on a configuration where FIXED_R18 is
redefined to 1.

We also have good build & tests results on aarch64-elf ports,
which don't redefine the macro.

I checked that we have uses of x18 in several places in libgnat.a
on aarch64-elf, both as a static chain and as a scratch, and
the only references to x18 in the VxWorks libgnat.a are from
functions referring to "environ", precisely the platform specific
use which prevents us from using the register otherwise.

I also sanity checked that "make all-gcc" passes (self tests
included) with mainline + the patch for --target=aarch64-elf
--enable-languages=c.

> Any reason not to use the TARGET_CONDITIONAL_REGISTER_USAGE interface to adjust the fixed_regs table instead?
> I'm not objecting to your approach here, just interested if you considered and rejected it.

Hmm, I haven't consciously rejected it. I just went for what
seemed natural at first sight from the elements in place.

In particular, the FIXED_REGS definition comes with a comment
on the use of r18 and it seemed logical to adjust both the comment
and the table there.

I'm happy to move that part to aarch64_conditional_register_usage
if that's considered more canonical of course.

It seems like I might need to set call_used_registers to 1 as well.

STATIC_CHAIN_REGNUM still needs to be adjusted directly I think.

I wondered if we could set it to R11 unconditionally and picked
the way ensuring no change for !vxworks ports, especially since I
don't have means to test more than what I described above.

We're working on a transition to gcc-8 and I can certainly port this
and the rest rapidly to verify that we get similar results on the
more recent code base. 

I'll be more than happy to incorporate suggestions of changes in
that process.

Thanks in advance,

With Kind Regards,

Olivier

Patch

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index fa9af26..2e69a4d 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -284,11 +284,13 @@  extern unsigned aarch64_architecture_version;
 
 /* Standard register usage.  */
 
+#define FIXED_R18 0
+
 /* 31 64-bit general purpose registers R0-R30:
    R30		LR (link register)
    R29		FP (frame pointer)
    R19-R28	Callee-saved registers
-   R18		The platform register; use as temporary register.
+   R18		The platform register; use as temporary register if !FIXED_R18.
    R17		IP1 The second intra-procedure-call temporary register
 		(can be used by call veneers and PLT code); otherwise use
 		as a temporary register
@@ -324,7 +326,7 @@  extern unsigned aarch64_architecture_version;
   {							\
     0, 0, 0, 0,   0, 0, 0, 0,	/* R0 - R7 */		\
     0, 0, 0, 0,   0, 0, 0, 0,	/* R8 - R15 */		\
-    0, 0, 0, 0,   0, 0, 0, 0,	/* R16 - R23 */		\
+    0, 0, FIXED_R18, 0, 0, 0, 0, 0,	/* R16 - R23 */		\
     0, 0, 0, 0,   0, 1, 0, 1,	/* R24 - R30, SP */	\
     0, 0, 0, 0,   0, 0, 0, 0,   /* V0 - V7 */           \
     0, 0, 0, 0,   0, 0, 0, 0,   /* V8 - V15 */		\
@@ -419,7 +421,7 @@  extern unsigned aarch64_architecture_version;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+#define STATIC_CHAIN_REGNUM		(!(FIXED_R18) ? R18_REGNUM : R11_REGNUM)
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM