| Message ID | 1B71130A-609B-40D9-9CE6-1014EED0C864@adacore.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ping] aarch64: move and adjust PROBE_STACK_*_REG | expand |
Ping, please ? Thanks in advance, Olivier > On 15 Oct 2020, at 08:38, Olivier Hainque <hainque@adacore.com> wrote: > > Ping, please ? > > Patch re-attached for convenience. > > Thanks in advance! > > Best Regards, > > Olivier > >> On 24 Sep 2020, at 11:46, Olivier Hainque <hainque@adacore.com> wrote: >> >> Re-proposing this patch after re-testing with a recent >> mainline on on aarch64-linux (bootstrap and regression test >> with --enable-languages=all), and more than a year of in-house >> use in production for a few aarch64 ports on a gcc-9 base. >> >> 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 adjusts >> 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). >> >> OK to commit? >> >> Thanks in advance, >> >> With Kind Regards, >> >> Olivier >> >> 2020-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. > > <aarch64-regnum.txt>
Hello, Another ping for this as a new end of stage1 approaches, please ? While this may ring the bell of a more involved issue with ABIs and the use of R18, this particular change doesn't have that kind of implication. Thanks a lot in advance! With Kind Regards, Olivier > On 26 Oct 2020, at 12:08, Olivier Hainque <hainque@adacore.com> wrote: > >> On 15 Oct 2020, at 08:38, Olivier Hainque <hainque@adacore.com> wrote: >> >>> On 24 Sep 2020, at 11:46, Olivier Hainque <hainque@adacore.com> wrote: >>> >>> Re-proposing this patch after re-testing with a recent >>> mainline on on aarch64-linux (bootstrap and regression test >>> with --enable-languages=all), and more than a year of in-house >>> use in production for a few aarch64 ports on a gcc-9 base. >>> >>> 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 adjusts >>> 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). >>> >>> OK to commit? >>> >>> Thanks in advance, >>> >>> With Kind Regards, >>> >>> Olivier >>> >>> 2020-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. >> >> <aarch64-regnum.txt> >
Olivier Hainque <hainque@adacore.com> writes: > Ping, please ? > > Patch re-attached for convenience. Looks OK to me, and I assume Richard would have spoken up by now if he didn't think the patch did what he wanted. > + ;; The pair of scratch registers used for stack probing with -fstack-check. > + ;; Leave R9 alone as a possible choice for the static chain. > + (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) It's a bit concerning that the second register now overlaps STACK_CLASH_SVE_CFA_REGNUM, but I agree that isn't a problem in practice, since the two uses are currently mutually-exclusive. I think it might be worth having a comment about that, So maybe add: ;; Note that the use of these registers is mutually exclusive with the use ;; of STACK_CLASH_SVE_CFA_REGNUM, which is for -fstack-clash-protection ;; rather than -fstack-check. to the new comment above. OK with that change, thanks. Sorry for the long delay in the review. Richard
Hi Richard, > On 4 Nov 2020, at 20:04, Richard Sandiford <richard.sandiford@arm.com> wrote: > > It's a bit concerning that the second register now overlaps > STACK_CLASH_SVE_CFA_REGNUM, but I agree that isn't a problem > in practice, since the two uses are currently mutually-exclusive. > I think it might be worth having a comment about that, So maybe add: > > ;; Note that the use of these registers is mutually exclusive with the use > ;; of STACK_CLASH_SVE_CFA_REGNUM, which is for -fstack-clash-protection > ;; rather than -fstack-check. > > to the new comment above. Sure. Yes, the two stack checking modes are definitely exclusive. > OK with that change, thanks. Sorry for the long delay in the review. Great :) No pb. Thanks for your feedback! Best Regards, Olivier
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index b6d74496cd0..5e94e4b1a79 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -6200,10 +6200,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. */ @@ -6217,7 +6213,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); @@ -6275,7 +6271,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 9b20dd0b1a0..df5533e113b 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 the static chain. + (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)