| Message ID | 20190116170338.GA19942@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [AArch64] Rename stack-clash CFA register to avoid clash. | expand |
On Wed, Jan 16, 2019 at 11:03:41AM -0600, Tamar Christina wrote: > Hi All, > > We had multiple patches in flight that required used of scratch registers in > frame layout code. As it happens two of these features picked the same register > and landed at around the same time. As such there is a clash when both are used > at the same time. This patch changes the temporary r15 to r11 for stack clash > and documents the "reserved" registers in the frame layout comment. > > Cross compiled and regtested on aarch64-none-elf with SVE on by default and no > issues. > Bootstrapped on aarch64-none-linux-gnu and no issues. > > Ok for trunk? My comments are all on your new comment detailing the register allocations, which I fully support. This patch is OK with those changes. > gcc/ChangeLog: > > 2019-01-16 Tamar Christina <tamar.christina@arm.com> > > PR target/88851 > * config/aarch64/aarch64.md (STACK_CLASH_SVE_CFA_REGNUM): New. > * config/aarch64/aarch64.c (aarch64_allocate_and_probe_stack_space): Use > it and document registers. > > gcc/testsuite/ChangeLog: > > 2019-01-16 Tamar Christina <tamar.christina@arm.com> > > PR target/88851 > * gcc.target/aarch64/stack-check-cfa-3.c: Update test. > > -- > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index fd60bddb1e1cbcb3dd46c319ccd182c7b9d1cd41..6a5f4956247b89932f955abbe96776a1b1ffb9cb 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -5317,11 +5317,11 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2, > { > /* This is done to provide unwinding information for the stack > adjustments we're about to do, however to prevent the optimizers > - from removing the R15 move and leaving the CFA note (which would be > + from removing the R11 move and leaving the CFA note (which would be > very wrong) we tie the old and new stack pointer together. > The tie will expand to nothing but the optimizers will not touch > the instruction. */ > - rtx stack_ptr_copy = gen_rtx_REG (Pmode, R15_REGNUM); > + rtx stack_ptr_copy = gen_rtx_REG (Pmode, STACK_CLASH_SVE_CFA_REGNUM); > emit_move_insn (stack_ptr_copy, stack_pointer_rtx); > emit_insn (gen_stack_tie (stack_ptr_copy, stack_pointer_rtx)); > > @@ -5548,7 +5548,18 @@ aarch64_add_cfa_expression (rtx_insn *insn, unsigned int reg, > to the stack we track as implicit probes are the FP/LR stores. > > For outgoing arguments we probe if the size is larger than 1KB, such that > - the ABI specified buffer is maintained for the next callee. */ > + the ABI specified buffer is maintained for the next callee. > + > + Aside from LR, FP, IP1 and IP0 there are a few other registers that if used > + would clash with other features: How about... The following registers are reserved during frame layout and should not be used for any other purpose. - LR <and so on> > + > + - r14 and r15: Used by mitigation code. "Used for speculation tracking." seems more correct. 'Mitigation Code' is too broad I think. > + - r16 and r17: Used by indirect tailcalls > + - r12 and r13: Used as temporaries for stack adjustment > + (EP0_REGNUM/EP1_REGNUM) > + - r11: Used by stack clash protection when SVE is enabled. Put them in numerical (or other logical) order? > + > + These registers should be avoided in frame layout related code. */ s/should/must/ > > /* Generate the prologue instructions for entry into a function. > Establish the stack frame by decreasing the stack pointer with a
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index fd60bddb1e1cbcb3dd46c319ccd182c7b9d1cd41..6a5f4956247b89932f955abbe96776a1b1ffb9cb 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -5317,11 +5317,11 @@ aarch64_allocate_and_probe_stack_space (rtx temp1, rtx temp2, { /* This is done to provide unwinding information for the stack adjustments we're about to do, however to prevent the optimizers - from removing the R15 move and leaving the CFA note (which would be + from removing the R11 move and leaving the CFA note (which would be very wrong) we tie the old and new stack pointer together. The tie will expand to nothing but the optimizers will not touch the instruction. */ - rtx stack_ptr_copy = gen_rtx_REG (Pmode, R15_REGNUM); + rtx stack_ptr_copy = gen_rtx_REG (Pmode, STACK_CLASH_SVE_CFA_REGNUM); emit_move_insn (stack_ptr_copy, stack_pointer_rtx); emit_insn (gen_stack_tie (stack_ptr_copy, stack_pointer_rtx)); @@ -5548,7 +5548,18 @@ aarch64_add_cfa_expression (rtx_insn *insn, unsigned int reg, to the stack we track as implicit probes are the FP/LR stores. For outgoing arguments we probe if the size is larger than 1KB, such that - the ABI specified buffer is maintained for the next callee. */ + the ABI specified buffer is maintained for the next callee. + + Aside from LR, FP, IP1 and IP0 there are a few other registers that if used + would clash with other features: + + - r14 and r15: Used by mitigation code. + - r16 and r17: Used by indirect tailcalls + - r12 and r13: Used as temporaries for stack adjustment + (EP0_REGNUM/EP1_REGNUM) + - r11: Used by stack clash protection when SVE is enabled. + + These registers should be avoided in frame layout related code. */ /* Generate the prologue instructions for entry into a function. Establish the stack frame by decreasing the stack pointer with a diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 513aec1872a7a5d29233d0bcb0bd1331d306eaaf..eb95a3c0935505e5c13c6e83a92b4e4db832ef94 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -35,15 +35,10 @@ (R11_REGNUM 11) (R12_REGNUM 12) (R13_REGNUM 13) - ;; Scratch registers for prologue/epilogue use. - (EP0_REGNUM 12) - (EP1_REGNUM 13) (R14_REGNUM 14) (R15_REGNUM 15) (R16_REGNUM 16) - (IP0_REGNUM 16) (R17_REGNUM 17) - (IP1_REGNUM 17) (R18_REGNUM 18) (R19_REGNUM 19) (R20_REGNUM 20) @@ -57,7 +52,6 @@ (R28_REGNUM 28) (R29_REGNUM 29) (R30_REGNUM 30) - (LR_REGNUM 30) (SP_REGNUM 31) (V0_REGNUM 32) (V1_REGNUM 33) @@ -113,10 +107,20 @@ (P13_REGNUM 81) (P14_REGNUM 82) (P15_REGNUM 83) + ;; Scratch register used by stack clash protection to calculate + ;; SVE CFA offsets during probing. + (STACK_CLASH_SVE_CFA_REGNUM 11) + ;; Scratch registers for prologue/epilogue use. + (EP0_REGNUM 12) + (EP1_REGNUM 13) ;; A couple of call-clobbered registers that we need to reserve when ;; tracking speculation this is not ABI, so is subject to change. - (SPECULATION_TRACKER_REGNUM 15) (SPECULATION_SCRATCH_REGNUM 14) + (SPECULATION_TRACKER_REGNUM 15) + ;; Scratch registers used in frame layout. + (IP0_REGNUM 16) + (IP1_REGNUM 17) + (LR_REGNUM 30) ] ) diff --git a/gcc/testsuite/gcc.target/aarch64/stack-check-cfa-3.c b/gcc/testsuite/gcc.target/aarch64/stack-check-cfa-3.c index 41579f26ba9156f3e500f090d132ba9cf28364d3..c4b7bb601c442a981ca309d0c3e8f29341b9b466 100644 --- a/gcc/testsuite/gcc.target/aarch64/stack-check-cfa-3.c +++ b/gcc/testsuite/gcc.target/aarch64/stack-check-cfa-3.c @@ -8,6 +8,6 @@ need to make sure we can unwind correctly before the frame is set up. So check that we're emitting r15 with a copy of sp an setting the CFA there. */ -/* { dg-final { scan-assembler-times {mov\tx15, sp} 1 } } */ -/* { dg-final { scan-assembler-times {\.cfi_def_cfa_register 15} 1 } } */ +/* { dg-final { scan-assembler-times {mov\tx11, sp} 1 } } */ +/* { dg-final { scan-assembler-times {\.cfi_def_cfa_register 11} 1 } } */ /* { dg-final { scan-assembler-times {\.cfi_escape 0xf,0xc,0x8f,0,0x92,0x2e,0,.*} 1 } } */