Re: [03/nn] [AArch64] Rework interface to add constant/offset routines

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Mon, Oct 30, 2017 at 10:52:26AM +0000, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@linaro.org> writes:
> > The port had aarch64_add_offset and aarch64_add_constant routines
> > that did similar things.  This patch replaces them with an expanded
> > version of aarch64_add_offset that takes separate source and
> > destination registers.  The new routine also takes a poly_int64 offset
> > instead of a HOST_WIDE_INT offset, but it leaves the HOST_WIDE_INT
> > case to aarch64_add_offset_1, which is basically a repurposed
> > aarch64_add_constant_internal.  The SVE patch will put the handling
> > of VL-based constants in aarch64_add_offset, while still using
> > aarch64_add_offset_1 for the constant part.
> >
> > The vcall_offset == 0 path in aarch64_output_mi_thunk will use temp0
> > as well as temp1 once SVE is added.
> >
> > A side-effect of the patch is that we now generate:
> >
> >         mov     x29, sp
> >
> > instead of:
> >
> >         add     x29, sp, 0
> >
> > in the pr70044.c test.
> 
> Sorry, I stupidly rebased the patch just before posting and so
> introduced a last-minute bug.  Here's a fixed version that survives
> testing on aarch64-linux-gnu.
>
> 2017-10-30  Richard Sandiford  <richard.sandiford@linaro.org>
> 	    Alan Hayward  <alan.hayward@arm.com>
> 	    David Sherwood  <david.sherwood@arm.com>
> 
> gcc/
> 	* config/aarch64/aarch64.c (aarch64_force_temporary): Assert that
> 	x exists before using it.
> 	(aarch64_add_constant_internal): Rename to...
> 	(aarch64_add_offset_1): ...this.  Replace regnum with separate
> 	src and dest rtxes.  Handle the case in which they're different,
> 	including when the offset is zero.  Replace scratchreg with an rtx.
> 	Use 2 additions if there is no spare register into which we can
> 	move a 16-bit constant.
> 	(aarch64_add_constant): Delete.
> 	(aarch64_add_offset): Replace reg with separate src and dest
> 	rtxes.  Take a poly_int64 offset instead of a HOST_WIDE_INT.
> 	Use aarch64_add_offset_1.
> 	(aarch64_add_sp, aarch64_sub_sp): Take the scratch register as
> 	an rtx rather than an int.  Take the delta as a poly_int64
> 	rather than a HOST_WIDE_INT.  Use aarch64_add_offset.
> 	(aarch64_expand_mov_immediate): Update uses of aarch64_add_offset.
> 	(aarch64_allocate_and_probe_stack_space): Take the scratch register
> 	as an rtx rather than an int.  Use Pmode rather than word_mode
> 	in the loop code.  Update calls to aarch64_sub_sp.
> 	(aarch64_expand_prologue): Update calls to aarch64_sub_sp,
> 	aarch64_allocate_and_probe_stack_space and aarch64_add_offset.
> 	(aarch64_expand_epilogue): Update calls to aarch64_add_offset
> 	and aarch64_add_sp.
> 	(aarch64_output_mi_thunk): Use aarch64_add_offset rather than
> 	aarch64_add_constant.
> 
> gcc/testsuite/
> 	* gcc.target/aarch64/pr70044.c: Allow "mov x29, sp" too.


> @@ -1966,86 +1949,123 @@ aarch64_internal_mov_immediate (rtx dest
>    return num_insns;
>  }
>  
> -/* Add DELTA to REGNUM in mode MODE.  SCRATCHREG can be used to hold a
> -   temporary value if necessary.  FRAME_RELATED_P should be true if
> -   the RTX_FRAME_RELATED flag should be set and CFA adjustments added
> -   to the generated instructions.  If SCRATCHREG is known to hold
> -   abs (delta), EMIT_MOVE_IMM can be set to false to avoid emitting the
> -   immediate again.
> -
> -   Since this function may be used to adjust the stack pointer, we must
> -   ensure that it cannot cause transient stack deallocation (for example
> -   by first incrementing SP and then decrementing when adjusting by a
> -   large immediate).  */
> +/* A subroutine of aarch64_add_offset that handles the case in which
> +   OFFSET is known at compile time.  The arguments are otherwise the same.  */

Some of the restrictions listed in this comment are important to keep
here. 

> -   Since this function may be used to adjust the stack pointer, we must
> -   ensure that it cannot cause transient stack deallocation (for example
> -   by first incrementing SP and then decrementing when adjusting by a
> -   large immediate).  */

This one in particular seems like we'd want it kept nearby the code.

OK with some sort of change to make the restrictions on what this code
should do clear on both functions.

Reviewed-by: James Greenhalgh <james.greenhalgh@arm.com>

Thanks,
James