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