[patch] follow up on the aarch64 r18 story

[patch] follow up on the aarch64 r18 story

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 2801 bytes --]

Hello,

About a year ago we discussed various possibilities regarding
an issue with the aarch64 selection of r18 as the static chain,
problematic in environments where the OS uses r18 for "private"
reasons. VxWorks uses this permission to use r18 as a pointer to
the current TCB, and Windows does something similar I think.

I first proposed a change allowing target ports to state that
r18 should be considered "fixed" and switching the static chain
to r11 in this case. I had picked r11 as the next after r9/r10
already used for stack checking,

After a few exchanges, we agreed that we should rather use r9
as the alternate static chain and remap the temporary registers
used for stack-checking to r10/r11.

  https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01592.html

We were also thinking that we should be able to change the
static chain register unconditionally.

Then during the last GNU cauldron, we realized that libffi
currently relies on the knowledge of which register is used for
the static chain, for the support of Go closures.

Unconditionally changing r18 to something else would break that
support, most notably on Linux. Adjusting libffi is possible but
raises a clear compatibility issue.

The attached patch is a renewed proposal to let target OS
ports state their private use of r18, switching the static
chain to r9 and declaring r18 "fixed" in that case. 

In addition to adjusting PROBE_STACK reg values to r10/r11,
the patch also moves the corresponding definitions together
with the other special register number definitions, so we have
a centralized view of the numbering choices.

This solves the immediate issue for VxWorks (which I'd like
to contribute) and seems like an interesting thing to have
in any case, regardless of whether or not we want to more
generally change the static chain someday.

With this change, we have good build and testing results
on both aarch64-elf and aarch64-vxworks7 for a gcc-9 based
compiler, where only vxworks states TARGET_OS_USES_R18.

I have also verified that I would build a native aarch64-linux
mainline compiler with the change and that it still picks r18 as
the static chain.

Is this variant ok to commit ?

Thanks in advance,

Best Regards,

Olivier

2019-11-07  Olivier Hainque  <hainque@adacore.com>

	* config/aarch64/aarch64.h: Define PROBE_STACK_FIRST_REG
	and PROBE_STACK_SECOND_REG here, to r10/r11 respectively.
	(TARGET_OS_USES_R18): New macro, default value 0 that target
	OS configuration files may redefine.
	(STATIC_CHAIN_REGNUM): r9 if TARGET_OS_USES_R18, r18 otherwise.
	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG,
	PROBE_STACK_SECOND_REG): Remove definitions.
	(aarch64_conditional_register_usage): Preserve r18 if the target
	OS uses it, and check that the static chain selection wouldn't
	conflict.


[-- Attachment #2: aarch64-r18.diff --]
[-- Type: application/octet-stream, Size: 2201 bytes --]

diff --git gcc/config/aarch64/aarch64.c gcc/config/aarch64/aarch64.c
index 1dfff33..36d2814 100644
--- gcc/config/aarch64/aarch64.c
+++ gcc/config/aarch64/aarch64.c
@@ -5435,10 +5435,6 @@ aarch64_libgcc_cmp_return_mode (void)
 #error Cannot use simple address calculation for stack probing
 #endif
 
-/* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  R9_REGNUM
-#define PROBE_STACK_SECOND_REG R10_REGNUM
-
 /* Emit code to probe a range of stack addresses from FIRST to FIRST+POLY_SIZE,
    inclusive.  These are offsets from the current stack pointer.  */
 
@@ -15527,6 +15523,14 @@ aarch64_conditional_register_usage (void)
       fixed_regs[SPECULATION_SCRATCH_REGNUM] = 1;
       call_used_regs[SPECULATION_SCRATCH_REGNUM] = 1;
     }
+
+  /* If the target OS has a private use of R18, preserve it.  */
+  if (TARGET_OS_USES_R18)
+    {
+      fixed_regs[R18_REGNUM] = 1;
+      call_used_regs[R18_REGNUM] = 1;
+      gcc_assert(STATIC_CHAIN_REGNUM != R18_REGNUM);
+    }
 }
 
 /* Walk down the type tree of TYPE counting consecutive base elements.
diff --git gcc/config/aarch64/aarch64.h gcc/config/aarch64/aarch64.h
index 425a363..3fccd2d 100644
--- gcc/config/aarch64/aarch64.h
+++ gcc/config/aarch64/aarch64.h
@@ -472,13 +472,22 @@ extern unsigned aarch64_architecture_version;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+/* Whether the target OS has a private use of R18, as allowed by the ABI.
+   OS specific configuration files may redefine this.  */
+#define TARGET_OS_USES_R18 0
+
+#define STATIC_CHAIN_REGNUM		(TARGET_OS_USES_R18 \
+					 ? R9_REGNUM : R18_REGNUM)
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM
 #define ARG_POINTER_REGNUM		AP_REGNUM
 #define FIRST_PSEUDO_REGISTER		(FFRT_REGNUM + 1)
 
+/* The pair of scratch registers used for stack probing during prologue.  */
+#define PROBE_STACK_FIRST_REG		R10_REGNUM
+#define PROBE_STACK_SECOND_REG		R11_REGNUM
+
 /* The number of argument registers available for each class.  */
 #define NUM_ARG_REGS			8
 #define NUM_FP_ARG_REGS			8

Re: [patch] follow up on the aarch64 r18 story

[Richard Earnshaw (lists)]

On 07/11/2019 16:16, Olivier Hainque wrote:
> Hello,
> 
> About a year ago we discussed various possibilities regarding
> an issue with the aarch64 selection of r18 as the static chain,
> problematic in environments where the OS uses r18 for "private"
> reasons. VxWorks uses this permission to use r18 as a pointer to
> the current TCB, and Windows does something similar I think.
> 
> I first proposed a change allowing target ports to state that
> r18 should be considered "fixed" and switching the static chain
> to r11 in this case. I had picked r11 as the next after r9/r10
> already used for stack checking,
> 
> After a few exchanges, we agreed that we should rather use r9
> as the alternate static chain and remap the temporary registers
> used for stack-checking to r10/r11.
> 
>    https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01592.html
> 
> We were also thinking that we should be able to change the
> static chain register unconditionally.
> 
> Then during the last GNU cauldron, we realized that libffi
> currently relies on the knowledge of which register is used for
> the static chain, for the support of Go closures.
> 
> Unconditionally changing r18 to something else would break that
> support, most notably on Linux. Adjusting libffi is possible but
> raises a clear compatibility issue.
> 
> The attached patch is a renewed proposal to let target OS
> ports state their private use of r18, switching the static
> chain to r9 and declaring r18 "fixed" in that case.
> 
> In addition to adjusting PROBE_STACK reg values to r10/r11,
> the patch also moves the corresponding definitions together
> with the other special register number definitions, so we have
> a centralized view of the numbering choices.
> 
> This solves the immediate issue for VxWorks (which I'd like
> to contribute) and seems like an interesting thing to have
> in any case, regardless of whether or not we want to more
> generally change the static chain someday.
> 
> With this change, we have good build and testing results
> on both aarch64-elf and aarch64-vxworks7 for a gcc-9 based
> compiler, where only vxworks states TARGET_OS_USES_R18.
> 
> I have also verified that I would build a native aarch64-linux
> mainline compiler with the change and that it still picks r18 as
> the static chain.
> 
> Is this variant ok to commit ?
> 
> Thanks in advance,
> 
> Best Regards,
> 
> Olivier
> 
> 2019-11-07  Olivier Hainque  <hainque@adacore.com>
> 
> 	* config/aarch64/aarch64.h: Define PROBE_STACK_FIRST_REG
> 	and PROBE_STACK_SECOND_REG here, to r10/r11 respectively.
> 	(TARGET_OS_USES_R18): New macro, default value 0 that target
> 	OS configuration files may redefine.
> 	(STATIC_CHAIN_REGNUM): r9 if TARGET_OS_USES_R18, r18 otherwise.
> 	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG,
> 	PROBE_STACK_SECOND_REG): Remove definitions.
> 	(aarch64_conditional_register_usage): Preserve r18 if the target
> 	OS uses it, and check that the static chain selection wouldn't
> 	conflict.
> 

My first thought is that if we need a permanent reservation it would be 
better to put this at the top of the list.  r16 and r17 have defined ABI 
uses (for the inter-procedural registers), so that means the next 
highest register is r15.  I'm keen that we avoid, if possible, 
fracturing the space of call clobbered registers by 'just picking 
another one' - if we're not careful we end up with a set of blocked 
holes that just makes life hard for everyone.

Which ever register we eventually settle on, I think this needs to get 
recorded in the ABI docs.  The GO example shows that this is more ABI 
than was previously anticipated.

+/* The pair of scratch registers used for stack probing during 
prologue.  */
+#define PROBE_STACK_FIRST_REG		R10_REGNUM
+#define PROBE_STACK_SECOND_REG		R11_REGNUM
+

These should be moved to the define_constant in aarch64.md that defines 
all the register numbers (add them to near the end of the list where the
other aliases are defined.

R.

Re: [patch] follow up on the aarch64 r18 story

[Olivier Hainque]

Thanks for your feedback on this Richard,

Back on this after a few days away.

> On 15 Nov 2019, at 16:37, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote:

> My first thought is that if we need a permanent reservation it would be better to put this at the top of the list.  r16 and r17 have defined ABI uses (for the inter-procedural registers), so that means the next highest register is r15.  I'm keen that we avoid, if possible, fracturing the space of call clobbered registers by 'just picking another one' - if we're not careful we end up with a set of blocked holes that just makes life hard for everyone.
> 
> Which ever register we eventually settle on, I think this needs to get recorded in the ABI docs. The GO example shows that this is more ABI than was previously anticipated.

Indeed. It might be just "normal" for something
essentially allowing access to local things through
part of of a Foreign Function Interface.

> +/* The pair of scratch registers used for stack probing during prologue.  */
> +#define PROBE_STACK_FIRST_REG		R10_REGNUM
> +#define PROBE_STACK_SECOND_REG		R11_REGNUM
> +
> 
> These should be moved to the define_constant in aarch64.md that defines all the register numbers (add them to near the end of the list where the
> other aliases are defined.

Sure, will adjust and retest. Thanks.

Best Regards,

Olivier

Re: [patch] follow up on the aarch64 r18 story

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

Hi Richard,

> On 21 Nov 2019, at 23:44, Olivier Hainque <hainque@adacore.com> wrote:

>> +/* The pair of scratch registers used for stack probing during prologue.  */
>> +#define PROBE_STACK_FIRST_REG		R10_REGNUM
>> +#define PROBE_STACK_SECOND_REG		R11_REGNUM
>> +
>> 
>> These should be moved to the define_constant in aarch64.md that defines all the register numbers (add them to near the end of the list where the
>> other aliases are defined.
> 
> Sure, will adjust and retest. Thanks.

Here's an updated version of the patch with your suggestion
incorporated, bootstrapped and regression checked for languages=all
on a native aarch64-linux host.

I replaced the _REG suffix by _REGNUM to match all the other
definitions in aarch64.md, just for consistency.

With Kind Regards,

Olivier

2019-11-07  Olivier Hainque  <hainque@adacore.com>

	* config/aarch64/aarch64.md: Define PROBE_STACK_FIRST_REGNUM
	and PROBE_STACK_SECOND_REGNUM constants, designating r10/r11.
	Replacements for the PROBE_STACK_FIRST/SECOND_REG constants in
	aarch64.c.
	* config/aarch64/aarch64.h (TARGET_OS_USES_R18): New macro,
	default value 0 that target OS configuration files may redefine.
	(STATIC_CHAIN_REGNUM): r9 if TARGET_OS_USES_R18, r18 otherwise.
	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG): Remove.
	(PROBE_STACK_SECOND_REG): Remove.
	(aarch64_emit_probe_stack_range): Adjust to the _REG -> _REGNUM
	suffix update for PROBE_STACK register numbers.
	(aarch64_conditional_register_usage): Preserve r18 if the target
	OS uses it, and check that the static chain selection wouldn't
	conflict.


[-- Attachment #2: aarch64-os-r18.diff --]
[-- Type: application/octet-stream, Size: 3176 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1dfff33..b992dc3 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5435,10 +5435,6 @@ aarch64_libgcc_cmp_return_mode (void)
 #error Cannot use simple address calculation for stack probing
 #endif
 
-/* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  R9_REGNUM
-#define PROBE_STACK_SECOND_REG R10_REGNUM
-
 /* Emit code to probe a range of stack addresses from FIRST to FIRST+POLY_SIZE,
    inclusive.  These are offsets from the current stack pointer.  */
 
@@ -5452,7 +5448,7 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, poly_int64 poly_size)
       return;
     }
 
-  rtx reg1 = gen_rtx_REG (Pmode, PROBE_STACK_FIRST_REG);
+  rtx reg1 = gen_rtx_REG (Pmode, PROBE_STACK_FIRST_REGNUM);
 
   /* See the same assertion on PROBE_INTERVAL above.  */
   gcc_assert ((first % ARITH_FACTOR) == 0);
@@ -5510,7 +5506,7 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, poly_int64 poly_size)
      equality test for the loop condition.  */
   else
     {
-      rtx reg2 = gen_rtx_REG (Pmode, PROBE_STACK_SECOND_REG);
+      rtx reg2 = gen_rtx_REG (Pmode, PROBE_STACK_SECOND_REGNUM);
 
       /* Step 1: round SIZE to the previous multiple of the interval.  */
 
@@ -15527,6 +15523,14 @@ aarch64_conditional_register_usage (void)
       fixed_regs[SPECULATION_SCRATCH_REGNUM] = 1;
       call_used_regs[SPECULATION_SCRATCH_REGNUM] = 1;
     }
+
+  /* If the target OS has a private use of R18, preserve it.  */
+  if (TARGET_OS_USES_R18)
+    {
+      fixed_regs[R18_REGNUM] = 1;
+      call_used_regs[R18_REGNUM] = 1;
+      gcc_assert(STATIC_CHAIN_REGNUM != R18_REGNUM);
+    }
 }
 
 /* Walk down the type tree of TYPE counting consecutive base elements.
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 425a363..904b347 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -472,7 +472,12 @@ extern unsigned aarch64_architecture_version;
    uses alloca.  */
 #define EXIT_IGNORE_STACK	(cfun->calls_alloca)
 
-#define STATIC_CHAIN_REGNUM		R18_REGNUM
+/* Whether the target OS has a private use of R18, as allowed by the ABI.
+   OS specific configuration files may redefine this.  */
+#define TARGET_OS_USES_R18 0
+
+#define STATIC_CHAIN_REGNUM		(TARGET_OS_USES_R18 \
+					 ? R9_REGNUM : R18_REGNUM)
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f19e227..b8cf4ab 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -117,6 +117,9 @@
     ;; Scratch registers for prologue/epilogue use.
     (EP0_REGNUM         12)
     (EP1_REGNUM         13)
+    ;; Scratch registers used for stack probing during prologue.
+    (PROBE_STACK_FIRST_REGNUM 10)
+    (PROBE_STACK_SECOND_REGNUM 11)
     ;; A couple of call-clobbered registers that we need to reserve when
     ;; tracking speculation this is not ABI, so is subject to change.
     (SPECULATION_SCRATCH_REGNUM 14)

[patch] move and adjust PROBE_STACK_*_REG on aarch64

[Olivier Hainque]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

Hello,

The attached patch is a standalone part of a slightly more
general change last discussed here:

  https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02626.html

Which I re-tested successfully on aarch64-linux (bootstrap
and regression test with --enable-languages=all + local tests
with Ada stack-checking on a gcc-9 based toolchain in-house).

The change moves the definitions of PROBE_STACK_FIRST_REG
and PROBE_STACK_SECOND_REG to a more appropriate place for such
items (here, in aarch64.md as suggested by Richard), and adjust
their value from r9/r10 to r10/r11 to free r9 for a possibly
more general purpose (e.g. as a static chain at least on targets
which have a private use of r18, such as Windows or Vxworks).

Is this ok to commit ?

Thanks in advance!

With Kind Regards,

Olivier

--

> 2019-11-07  Olivier Hainque  <hainque@adacore.com>
> 
> 	* config/aarch64/aarch64.md: Define PROBE_STACK_FIRST_REGNUM
> 	and PROBE_STACK_SECOND_REGNUM constants, designating r10/r11.
> 	Replacements for the PROBE_STACK_FIRST/SECOND_REG constants in
> 	aarch64.c.
> 	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG): Remove.
> 	(PROBE_STACK_SECOND_REG): Remove.
> 	(aarch64_emit_probe_stack_range): Adjust to the _REG -> _REGNUM
> 	suffix update for PROBE_STACK register numbers.


[-- Attachment #2: 0001-Move-and-adjust-PROBE_STACK-reg-definitions-for-aarc.patch.txt --]
[-- Type: text/plain, Size: 2098 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1dfff33..b5a2815 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5435,10 +5435,6 @@ aarch64_libgcc_cmp_return_mode (void)
 #error Cannot use simple address calculation for stack probing
 #endif
 
-/* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  R9_REGNUM
-#define PROBE_STACK_SECOND_REG R10_REGNUM
-
 /* Emit code to probe a range of stack addresses from FIRST to FIRST+POLY_SIZE,
    inclusive.  These are offsets from the current stack pointer.  */
 
@@ -5452,7 +5448,7 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, poly_int64 poly_size)
       return;
     }
 
-  rtx reg1 = gen_rtx_REG (Pmode, PROBE_STACK_FIRST_REG);
+  rtx reg1 = gen_rtx_REG (Pmode, PROBE_STACK_FIRST_REGNUM);
 
   /* See the same assertion on PROBE_INTERVAL above.  */
   gcc_assert ((first % ARITH_FACTOR) == 0);
@@ -5510,7 +5506,7 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, poly_int64 poly_size)
      equality test for the loop condition.  */
   else
     {
-      rtx reg2 = gen_rtx_REG (Pmode, PROBE_STACK_SECOND_REG);
+      rtx reg2 = gen_rtx_REG (Pmode, PROBE_STACK_SECOND_REGNUM);
 
       /* Step 1: round SIZE to the previous multiple of the interval.  */
 
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f19e227..2775d75e 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -111,6 +111,10 @@
     ;; "FFR token": a fake register used for representing the scheduling
     ;; restrictions on FFR-related operations.
     (FFRT_REGNUM	85)
+    ;; The pair of scratch registers used for stack probing with -fstack-check.
+    ;; Leave R9 alone as a possible choice for more general purposes.
+    (PROBE_STACK_FIRST_REGNUM  10)
+    (PROBE_STACK_SECOND_REGNUM 11)
     ;; Scratch register used by stack clash protection to calculate
     ;; SVE CFA offsets during probing.
     (STACK_CLASH_SVE_CFA_REGNUM 11)
-- 
1.9.1

[ping] move and adjust PROBE_STACK_*_REG on aarch64

[Olivier Hainque]

Hello,

Gentle ping for 

  https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01398.html

I realize and certainly understand that this is
of lower importance than other matters currently being discussed
on the lists.

It's just that the end of stage 3 is imminent and I thought
we'd better at least agree on a way forward before it closes.

This particular change is an extraction of an hopefully
non controversial part of a more general change discussed
earlier. No ABI concern here.

Thanks in advance!

Cheers,

Olivier

> On 19 Dec 2019, at 18:38, Olivier Hainque <hainque@adacore.com> wrote:
> 
> Hello,
> 
> The attached patch is a standalone part of a slightly more
> general change last discussed here:
> 
>  https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02626.html
> 
> Which I re-tested successfully on aarch64-linux (bootstrap
> and regression test with --enable-languages=all + local tests
> with Ada stack-checking on a gcc-9 based toolchain in-house).
> 
> The change moves the definitions of PROBE_STACK_FIRST_REG
> and PROBE_STACK_SECOND_REG to a more appropriate place for such
> items (here, in aarch64.md as suggested by Richard), and adjust
> their value from r9/r10 to r10/r11 to free r9 for a possibly
> more general purpose (e.g. as a static chain at least on targets
> which have a private use of r18, such as Windows or Vxworks).
> 
> Is this ok to commit ?
> 
> Thanks in advance!
> 
> With Kind Regards,
> 
> Olivier
> 
> --
> 
>> 2019-11-07  Olivier Hainque  <hainque@adacore.com>
>> 
>> 	* config/aarch64/aarch64.md: Define PROBE_STACK_FIRST_REGNUM
>> 	and PROBE_STACK_SECOND_REGNUM constants, designating r10/r11.
>> 	Replacements for the PROBE_STACK_FIRST/SECOND_REG constants in
>> 	aarch64.c.
>> 	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG): Remove.
>> 	(PROBE_STACK_SECOND_REG): Remove.
>> 	(aarch64_emit_probe_stack_range): Adjust to the _REG -> _REGNUM
>> 	suffix update for PROBE_STACK register numbers.
> 
> <0001-Move-and-adjust-PROBE_STACK-reg-definitions-for-aarc.patch.txt>