Message ID | 3FA70C04-A7B3-4C74-AFF7-7BAC7C2C0DBC@adacore.com |
---|---|
State | New |
Headers | show |
Series | allow target config to state r18 is fixed on aarch64 | expand |
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
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