allow target config to state r18 is fixed on aarch64

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.

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

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

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

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

> 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

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

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