Re: [patch] allow target config to state r18 is fixed on aarch64

Re: [patch] allow target config to state r18 is fixed on aarch64

[Wilco Dijkstra]

Hi Olivier,

> 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.

Yes it should always be the same register, there is no gain in switching
it dynamically. I'd suggest to use X9 since X8 is the last register used for
arguments (STATIC_CHAIN_REGNUM is passed when calling a nested
function) and some of the higher registers may be used as temporaries in
prolog/epilog.

Wilco

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

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

Hi Wilco,

> On 18 Oct 2018, at 19:08, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

>> 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.
> 
> Yes it should always be the same register, there is no gain in switching
> it dynamically. I'd suggest to use X9 since X8 is the last register used for
> arguments (STATIC_CHAIN_REGNUM is passed when calling a nested
> function) and some of the higher registers may be used as temporaries in
> prolog/epilog.

Thanks for your feedback!  I ported the patches
to gcc-8 and was able to get a functional toolchain
for aarch64-wrs-vxworks7 and aarch64-elf, passing
full Acats for a couple of runtime variants on VxWorks
(compiled for RTP or kernel mode) as well as a small
internal testsuite we have, dedicated to cross configurations.

All the patches apply directly on mainline.

As for the original patch, I also sanity checked that
"make all-gcc" passes (self tests included) there for
--target=aarch64-elf --enable-languages=c


There are three changes to the common aarch64 port files.

It turns out that X9 doesn't work for STATIC_CHECK_REGNUM
because this conflicts with the registers used for -fstack-check:

  /* The pair of scratch registers used for stack probing.  */
  #define PROBE_STACK_FIRST_REG  9
  #define PROBE_STACK_SECOND_REG 10

I didn't find that immediately (read, I first experienced a few
badly crashing test runs) because I searched for R9_REGNUM to check
for other uses, so the first patchlet attached simply adjusts
the two #defines above to use R9/R10_REGNUM.


2018-10-23  Olivier Hainque  <hainque@adacore.com>

        * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
        R9_REGNUM instead of 9.
        (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.


The second patch is the one which I proposed a few days ago
to allow a subtarget (in my case, the VxWorks port) to state that
R18 is to be considered fixed. Two changes compared to the original
patch: a comment attached to the default definition of FIXED_R18,
and the unconditional use of R11_REGNUM as an alternate STATIC_CHAIN_REGNUM.

I suppose the latter could require extra testing than what I was
able to put in (since this is also changing for !vxworks configurations),
which Sam very kindly did on the first instance.

I didn't touch CALL_SAVED_REGISTERS since this is 1 for r18 already.
I also didn't see a strong reason to move to a more dynamic scheme,
through conditional_register_usage.

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): Use r11 instead of r18.


The third patch proposes the introduction of support for a
conditional SUBTARGET_OVERRIDE_OPTIONS macro, as many other
architectures have, and which is needed by all VxWorks ports.

In the current state, this one could possibly impact only
VxWorks, as no other config file would define the macro.

I'm not 100% clear on the possible existence of rules regarding
the placement of this within the override_options functions. We used
something similar to what other ports do, and it worked just fine
for VxWorks.

2018-10-23  Olivier Hainque  <hainque@adacore.com>

        * config/aarch64/aarch64.c (aarch64_override_options): Once
        arch, cpu and tune were validated, insert SUBTARGET_OVERRIDE_OPTIONS
        if defined.

I'm happy to adjust any of all this if needed of course.

Thanks in advance for your feedback!

With Kind Regards,

Olivier



[-- Attachment #2: 0001-Use-R9-R10_REGNUM-to-designate-stack-checking-regist.patch.txt --]
[-- Type: text/plain, Size: 980 bytes --]

From dd32f3611e4cb10f0b48d58d84c96951befbd99f Mon Sep 17 00:00:00 2001
From: Olivier Hainque <hainque@adacore.com>
Date: Sun, 21 Oct 2018 10:32:13 +0200
Subject: [PATCH 1/6] Use R9/R10_REGNUM to designate stack checking registers

---
 gcc/config/aarch64/aarch64.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 6810db7..f03e803 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3793,8 +3793,8 @@ aarch64_libgcc_cmp_return_mode (void)
 #endif
 
 /* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  9
-#define PROBE_STACK_SECOND_REG 10
+#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.  */
-- 
1.7.10.4


[-- Attachment #3: 0002-Introduce-FIXED_R18-in-aarch64.h.patch.txt --]
[-- Type: text/plain, Size: 2133 bytes --]

From 399f0f4773bc3771f2a03c1e750f3b09d6b4824f Mon Sep 17 00:00:00 2001
From: Olivier Hainque <hainque@adacore.com>
Date: Fri, 19 Oct 2018 12:54:26 +0200
Subject: [PATCH 2/6] Introduce FIXED_R18 in aarch64.h

---
 gcc/config/aarch64/aarch64.h |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 976f9af..4069e29 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -267,11 +267,17 @@ extern unsigned aarch64_architecture_version;
 
 /* Standard register usage.  */
 
+/* Whether R18, the "platform register", is used as such by the target
+   environment, and may thus not be used as a scratch register.  This may
+   be redefined to 1 by os specific configurations.  */
+#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; available as temporary register
+                unless FIXED_R18 is set.
    R17		IP1 The second intra-procedure-call temporary register
 		(can be used by call veneers and PLT code); otherwise use
 		as a temporary register
@@ -316,7 +322,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 */		\
@@ -403,7 +409,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		R11_REGNUM
 #define HARD_FRAME_POINTER_REGNUM	R29_REGNUM
 #define FRAME_POINTER_REGNUM		SFP_REGNUM
 #define STACK_POINTER_REGNUM		SP_REGNUM
-- 
1.7.10.4


[-- Attachment #4: 0003-Support-for-SUBTARGET_OVERRIDE_OPTIONS-in-aarch64.c.patch.txt --]
[-- Type: text/plain, Size: 952 bytes --]

From 67ffe5c97d19e3a4aa04e3f9de92b77346988870 Mon Sep 17 00:00:00 2001
From: Olivier Hainque <hainque@adacore.com>
Date: Fri, 19 Oct 2018 21:19:52 +0200
Subject: [PATCH 3/6] Support for SUBTARGET_OVERRIDE_OPTIONS in aarch64.c

---
 gcc/config/aarch64/aarch64.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index f03e803..b4fedbd 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10893,6 +10893,10 @@ aarch64_override_options (void)
   if (aarch64_tune_string)
     valid_tune = aarch64_validate_mtune (aarch64_tune_string, &selected_tune);
 
+#ifdef SUBTARGET_OVERRIDE_OPTIONS
+  SUBTARGET_OVERRIDE_OPTIONS;
+#endif
+
   /* If the user did not specify a processor, choose the default
      one for them.  This will be the CPU set during configuration using
      --with-cpu, otherwise it is "generic".  */
-- 
1.7.10.4

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

Hello Wilco,

Would you have further thoughts on the patches proposed in 

 https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01453.html

?

There was:

1)  * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
    R9_REGNUM instead of 9.
    (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.


2)  * config/aarch64/aarch64.h (FIXED_R18): New internal
    configuration macro, defaulted to 0.
    (FIXED_REGISTERS): Use it.
    (STATIC_CHAIN_REGNUM): Use r11 instead of r18.

and

3)  * config/aarch64/aarch64.c (aarch64_override_options): Once
    arch, cpu and tune were validated, insert SUBTARGET_OVERRIDE_OPTIONS
   if defined.



Thanks a lot in advance!

With Kind Regards,

Olivier


> On 23 Oct 2018, at 19:13, Olivier Hainque <hainque@adacore.com> wrote:
> 
> Hi Wilco,
> 
>> On 18 Oct 2018, at 19:08, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> 
>>> 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.
>> 
>> Yes it should always be the same register, there is no gain in switching
>> it dynamically. I'd suggest to use X9 since X8 is the last register used for
>> arguments (STATIC_CHAIN_REGNUM is passed when calling a nested
>> function) and some of the higher registers may be used as temporaries in
>> prolog/epilog.
> 
> Thanks for your feedback!  I ported the patches
> to gcc-8 and was able to get a functional toolchain
> for aarch64-wrs-vxworks7 and aarch64-elf, passing
> full Acats for a couple of runtime variants on VxWorks
> (compiled for RTP or kernel mode) as well as a small
> internal testsuite we have, dedicated to cross configurations.
> 
> All the patches apply directly on mainline.
> 
> As for the original patch, I also sanity checked that
> "make all-gcc" passes (self tests included) there for
> --target=aarch64-elf --enable-languages=c
> 
> 
> There are three changes to the common aarch64 port files.
> 
> It turns out that X9 doesn't work for STATIC_CHECK_REGNUM
> because this conflicts with the registers used for -fstack-check:
> 
>  /* The pair of scratch registers used for stack probing.  */
>  #define PROBE_STACK_FIRST_REG  9
>  #define PROBE_STACK_SECOND_REG 10
> 
> I didn't find that immediately (read, I first experienced a few
> badly crashing test runs) because I searched for R9_REGNUM to check
> for other uses, so the first patchlet attached simply adjusts
> the two #defines above to use R9/R10_REGNUM.
> 
> 
> 2018-10-23  Olivier Hainque  <hainque@adacore.com>
> 
>        * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
>        R9_REGNUM instead of 9.
>        (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.
> 
> 
> The second patch is the one which I proposed a few days ago
> to allow a subtarget (in my case, the VxWorks port) to state that
> R18 is to be considered fixed. Two changes compared to the original
> patch: a comment attached to the default definition of FIXED_R18,
> and the unconditional use of R11_REGNUM as an alternate STATIC_CHAIN_REGNUM.
> 
> I suppose the latter could require extra testing than what I was
> able to put in (since this is also changing for !vxworks configurations),
> which Sam very kindly did on the first instance.
> 
> I didn't touch CALL_SAVED_REGISTERS since this is 1 for r18 already.
> I also didn't see a strong reason to move to a more dynamic scheme,
> through conditional_register_usage.
> 
> 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): Use r11 instead of r18.
> 
> 
> The third patch proposes the introduction of support for a
> conditional SUBTARGET_OVERRIDE_OPTIONS macro, as many other
> architectures have, and which is needed by all VxWorks ports.
> 
> In the current state, this one could possibly impact only
> VxWorks, as no other config file would define the macro.
> 
> I'm not 100% clear on the possible existence of rules regarding
> the placement of this within the override_options functions. We used
> something similar to what other ports do, and it worked just fine
> for VxWorks.
> 
> 2018-10-23  Olivier Hainque  <hainque@adacore.com>
> 
>        * config/aarch64/aarch64.c (aarch64_override_options): Once
>        arch, cpu and tune were validated, insert SUBTARGET_OVERRIDE_OPTIONS
>        if defined.
> 
> I'm happy to adjust any of all this if needed of course.
> 
> Thanks in advance for your feedback!
> 
> With Kind Regards,
> 
> Olivier
> 
> 
> <0001-Use-R9-R10_REGNUM-to-designate-stack-checking-regist.patch.txt><0002-Introduce-FIXED_R18-in-aarch64.h.patch.txt><0003-Support-for-SUBTARGET_OVERRIDE_OPTIONS-in-aarch64.c.patch.txt>

Re: [patch] allow target config to state r18 is fixed on aarch64

[Andrew Pinski]

On Fri, Nov 9, 2018 at 10:09 AM Olivier Hainque <hainque@adacore.com> wrote:
>
> Hello Wilco,
>
> Would you have further thoughts on the patches proposed in
>
>  https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01453.html
>
> ?
>
> There was:
>
> 1)  * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
>     R9_REGNUM instead of 9.
>     (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.
>
>
> 2)  * config/aarch64/aarch64.h (FIXED_R18): New internal
>     configuration macro, defaulted to 0.
>     (FIXED_REGISTERS): Use it.
>     (STATIC_CHAIN_REGNUM): Use r11 instead of r18.
>
> and
>
> 3)  * config/aarch64/aarch64.c (aarch64_override_options): Once
>     arch, cpu and tune were validated, insert SUBTARGET_OVERRIDE_OPTIONS
>    if defined.

I remember hearing from some folks, they would like this (the ability
to mark r18 as being fixed) to be a target option so they could
support wine on AARCH64 too; I hear clang has this option already too.
https://bugs.winehq.org/show_bug.cgi?id=38780 for the wine issue.

Thanks,
Andrew

>
>
>
> Thanks a lot in advance!
>
> With Kind Regards,
>
> Olivier
>
>
> > On 23 Oct 2018, at 19:13, Olivier Hainque <hainque@adacore.com> wrote:
> >
> > Hi Wilco,
> >
> >> On 18 Oct 2018, at 19:08, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> >
> >>> 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.
> >>
> >> Yes it should always be the same register, there is no gain in switching
> >> it dynamically. I'd suggest to use X9 since X8 is the last register used for
> >> arguments (STATIC_CHAIN_REGNUM is passed when calling a nested
> >> function) and some of the higher registers may be used as temporaries in
> >> prolog/epilog.
> >
> > Thanks for your feedback!  I ported the patches
> > to gcc-8 and was able to get a functional toolchain
> > for aarch64-wrs-vxworks7 and aarch64-elf, passing
> > full Acats for a couple of runtime variants on VxWorks
> > (compiled for RTP or kernel mode) as well as a small
> > internal testsuite we have, dedicated to cross configurations.
> >
> > All the patches apply directly on mainline.
> >
> > As for the original patch, I also sanity checked that
> > "make all-gcc" passes (self tests included) there for
> > --target=aarch64-elf --enable-languages=c
> >
> >
> > There are three changes to the common aarch64 port files.
> >
> > It turns out that X9 doesn't work for STATIC_CHECK_REGNUM
> > because this conflicts with the registers used for -fstack-check:
> >
> >  /* The pair of scratch registers used for stack probing.  */
> >  #define PROBE_STACK_FIRST_REG  9
> >  #define PROBE_STACK_SECOND_REG 10
> >
> > I didn't find that immediately (read, I first experienced a few
> > badly crashing test runs) because I searched for R9_REGNUM to check
> > for other uses, so the first patchlet attached simply adjusts
> > the two #defines above to use R9/R10_REGNUM.
> >
> >
> > 2018-10-23  Olivier Hainque  <hainque@adacore.com>
> >
> >        * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
> >        R9_REGNUM instead of 9.
> >        (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.
> >
> >
> > The second patch is the one which I proposed a few days ago
> > to allow a subtarget (in my case, the VxWorks port) to state that
> > R18 is to be considered fixed. Two changes compared to the original
> > patch: a comment attached to the default definition of FIXED_R18,
> > and the unconditional use of R11_REGNUM as an alternate STATIC_CHAIN_REGNUM.
> >
> > I suppose the latter could require extra testing than what I was
> > able to put in (since this is also changing for !vxworks configurations),
> > which Sam very kindly did on the first instance.
> >
> > I didn't touch CALL_SAVED_REGISTERS since this is 1 for r18 already.
> > I also didn't see a strong reason to move to a more dynamic scheme,
> > through conditional_register_usage.
> >
> > 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): Use r11 instead of r18.
> >
> >
> > The third patch proposes the introduction of support for a
> > conditional SUBTARGET_OVERRIDE_OPTIONS macro, as many other
> > architectures have, and which is needed by all VxWorks ports.
> >
> > In the current state, this one could possibly impact only
> > VxWorks, as no other config file would define the macro.
> >
> > I'm not 100% clear on the possible existence of rules regarding
> > the placement of this within the override_options functions. We used
> > something similar to what other ports do, and it worked just fine
> > for VxWorks.
> >
> > 2018-10-23  Olivier Hainque  <hainque@adacore.com>
> >
> >        * config/aarch64/aarch64.c (aarch64_override_options): Once
> >        arch, cpu and tune were validated, insert SUBTARGET_OVERRIDE_OPTIONS
> >        if defined.
> >
> > I'm happy to adjust any of all this if needed of course.
> >
> > Thanks in advance for your feedback!
> >
> > With Kind Regards,
> >
> > Olivier
> >
> >
> > <0001-Use-R9-R10_REGNUM-to-designate-stack-checking-regist.patch.txt><0002-Introduce-FIXED_R18-in-aarch64.h.patch.txt><0003-Support-for-SUBTARGET_OVERRIDE_OPTIONS-in-aarch64.c.patch.txt>
>

[ping] use REGNUM macros instead of hardcoded values in aarch64 PROBE_STACK reg definitions

[Olivier Hainque]

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

Hello,

Ping for one of the changes last proposed here:

  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00761.html

Submitted separately as an attempt to facilitate the review
process.

Got access to a linux box, so in addition to the Ada testing
we do on cross aarch64-vxworks7, bootstrapped and regression
tested on aarch64-linux.


This one is just a simple use of REGNUM macros instead
of harcoded values in the definition of PROBE_STACK_FIRST_REG
and PROBE_STACK_SECOND_REG in aarch64.c.

OK to commit ?

Thanks in advance!

Olivier

2018-12-12  Olivier Hainque  <hainque@adacore.com>

	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
	R9_REGNUM instead of 9.
	(PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.


[-- Attachment #2: 0001-Use-REGNUM-macros-in-the-definitions-of-aarch64-PROB.patch --]
[-- Type: application/octet-stream, Size: 1144 bytes --]

From 02518d7099c8376c7de67c90a59735199aa660bf Mon Sep 17 00:00:00 2001
From: Olivier Hainque <hainque@adacore.com>
Date: Wed, 12 Dec 2018 06:49:59 -0800
Subject: [PATCH 1/4] Use REGNUM macros in the definitions of aarch64
 PROBE_STACK regs

	* config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
	R9_REGNUM instead of 9.
	(PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.
---
 gcc/config/aarch64/aarch64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 3d597ca..891de75 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -3896,8 +3896,8 @@ aarch64_libgcc_cmp_return_mode (void)
 #endif
 
 /* The pair of scratch registers used for stack probing.  */
-#define PROBE_STACK_FIRST_REG  9
-#define PROBE_STACK_SECOND_REG 10
+#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.  */
-- 
1.9.1

Re: [ping] use REGNUM macros instead of hardcoded values in aarch64 PROBE_STACK reg definitions

[Kyrill Tkachov]

Hi Olivier,

On 12/12/18 15:35, Olivier Hainque wrote:
> Hello,
>
> Ping for one of the changes last proposed here:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00761.html
>
> Submitted separately as an attempt to facilitate the review
> process.
>
> Got access to a linux box, so in addition to the Ada testing
> we do on cross aarch64-vxworks7, bootstrapped and regression
> tested on aarch64-linux.
>
>
> This one is just a simple use of REGNUM macros instead
> of harcoded values in the definition of PROBE_STACK_FIRST_REG
> and PROBE_STACK_SECOND_REG in aarch64.c.
>
> OK to commit ?
>

This looks obvious to me tbh, I'd go ahead and commit it.
Thanks,
Kyrill

> Thanks in advance!
>
> Olivier
>
> 2018-12-12  Olivier Hainque  <hainque@adacore.com>
>
>         * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
>         R9_REGNUM instead of 9.
>         (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.
>

Re: [ping] use REGNUM macros instead of hardcoded values in aarch64 PROBE_STACK reg definitions

[Olivier Hainque]

Hi Kyrill,

> On 12 Dec 2018, at 16:43, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> This looks obvious to me tbh, I'd go ahead and commit it.

Great :-)

I have just sent separate updates on the other, less obvious
parts (SUBTARGET_OVERRIDE_OPTIONS, static chain change to r11,
and FIXED_R18).

Thanks!

>>        * config/aarch64/aarch64.c (PROBE_STACK_FIRST_REG) : Redefine as
>>        R9_REGNUM instead of 9.
>>        (PROBE_STACK_SECOND_REG): Redefine as R10_REGNUM instead of 10.

[patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

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

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.


[-- Attachment #2: fixed-r18.txt --]
[-- Type: text/plain, Size: 1547 bytes --]

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

Re: [patch] allow target config to state r18 is fixed on aarch64

[Kyrill Tkachov]

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

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

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

Re: [patch] allow target config to state r18 is fixed on aarch64

[Kyrill Tkachov]

Hi Olivier,

On 12/10/18 19:43, Olivier Hainque wrote:
> 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.

I don't think it's more canonical, and it is a run-time thing, whereas your patch changes things
at configure time, so there's no runtime overhead.

> 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 think so too, so you'd still need to have these configure-time changes.
If we could make it all runtime that would be clean, but perhaps it's not worth
splicing the two approaches.

> 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.

So, do you still want to make this change in current trunk? Or will you make the necessary changes
when contributing the vxworks port?

Thanks,
Kyrill

> I'll be more than happy to incorporate suggestions of changes in
> that process.
>
> Thanks in advance,
>
> With Kind Regards,
>
> Olivier
>
>

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

Hi Kyrill,

> On 16 Oct 2018, at 18:33, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
>> I'm happy to move that part to aarch64_conditional_register_usage
>> if that's considered more canonical of course.
> 
> I don't think it's more canonical, and it is a run-time thing, whereas your patch changes things
> at configure time, so there's no runtime overhead.

Ok.

>> 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 think so too, so you'd still need to have these configure-time changes.
> If we could make it all runtime that would be clean, but perhaps it's not worth
> splicing the two approaches.

Agreed. I also thought of triggering the effect of -ffixed-r18
from within one of the vxworks early hooks, but this wouldn't prevent
the use as a static chain AFAICS. Interesting ...

>> 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.
> 
> So, do you still want to make this change in current trunk? Or will you make the necessary changes
> when contributing the vxworks port?

I'm working on the port and will re propose
the change as part of that soon, hopefully in the
forthcoming days.

It was very useful to have early feedback on the approach,
thanks !

Olivier

Re: [patch] allow target config to state r18 is fixed on aarch64

[Sam Tebbs]

On 10/12/2018 07:43 PM, Olivier Hainque wrote:
>> 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.

Hi Olivier,

I managed to run a bootstrap build with your patch applied on one of our 
aarch64 machines, so there shouldn't be any problems in that regard.

Regards,
Sam

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

> On 18 Oct 2018, at 14:14, Sam Tebbs <Sam.Tebbs@arm.com> wrote:
> 
> 
> 
> On 10/12/2018 07:43 PM, Olivier Hainque wrote:
>>> 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.
> 
> Hi Olivier,
> 
> I managed to run a bootstrap build with your patch applied on one of our aarch64 machines, so there shouldn't be any problems in that regard.

Great ! Thanks a lot Sam ! Much appreciated.

I'm currently building our vxworks port on a gcc-8 source
base and will then most probably retain the static approach
instead of going runtime.

The only difference there would be wrt to this part
is the use of the macro within called_used_regs[] as well,
part of what we discussed with Kyrill.

I'll reapply my testing, and with the one you just made on
top, I think the syntax / logic of the change is straightforward
enough to have confidence.

With Kind Regards,

Olivier

Re: [patch] allow target config to state r18 is fixed on aarch64

[Olivier Hainque]

> On 18 Oct 2018, at 15:10, Olivier Hainque <hainque@adacore.com> wrote:
> 
> The only difference there would be wrt to this part
> is the use of the macro within called_used_regs[] as well,
> part of what we discussed with Kyrill.

Ah, no, call_used[r18] is 1 currently.

Will give this some thought ...