Re: [AArch64][3/3] Migrate aarch64_expand_prologue/epilogue to aarch64_add_constant

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 20/07/16 16:02, Jiong Wang wrote:
> On 20/07/16 15:18, Richard Earnshaw (lists) wrote:
>> On 20/07/16 14:03, Jiong Wang wrote:
>>> Those stack adjustment sequences inside aarch64_expand_prologue/epilogue
>>> are doing exactly what's aarch64_add_constant offered, except they also
>>> need to be aware of dwarf generation.
>>>
>>> This patch teach existed aarch64_add_constant about dwarf generation and
>>> currently SP register is supported.  Whenever SP is updated, there
>>> should be CFA update, we then mark these instructions as frame related,
>>> and if the update is too complex for gcc to guess the adjustment, we
>>> attach explicit annotation.
>>>
>>> Both dwarf frame info size and pro/epilogue scheduling are improved
>>> after
>>> this patch as aarch64_add_constant has better utilization of scratch
>>> register.
>>>
>>> OK for trunk?
>>>
>>> gcc/
>>> 2016-07-20  Jiong Wang  <jiong.wang@arm.com>
>>>
>>>              * config/aarch64/aarch64.c (aarch64_add_constant): Mark
>>>              instruction as frame related when it is.  Generate CFA
>>>              annotation when it's necessary.
>>>              (aarch64_expand_prologue): Use aarch64_add_constant.
>>>              (aarch64_expand_epilogue): Likewise.
>>>
>> Are you sure using aarch64_add_constant is unconditionally safe?  Stack
>> adjustments need to be done very carefully to ensure that we never
>> transiently deallocate part of the stack.
> 
> Richard,
> 
>   Thanks for the review, yes, I believe using aarch64_add_constant is
> unconditionally
> safe here.  Because we have generated a stack tie to clobber the whole
> memory thus
> prevent any instruction which access stack be scheduled after that.
> 
>   The access to deallocated stack issue was there and fixed by
> 
>   https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02292.html.
> 
>  aarch64_add_constant itself is generating the same instruction
> sequences as the
> original code, except for a few cases, it will prefer
> 
>   move scratch_reg, #imm
>   add sp, sp, scratch_reg
> 
> than:
>   add sp, sp, #imm_part1
>   add sp, sp, #imm_part2
> 
> 
> 
> 

OK, I've had another look at this and I'm happy that we don't
(currently) run into the problem I'm concerned about.

However, this new usage does impose a constraint on aarch64_add_constant
that will need to be respected in future, so please can you add the
following to the comment that precedes that function:

/* ...

   This function is sometimes used to adjust the stack pointer, so we
   must ensure that it can never cause transient stack deallocation
   by writing an invalid value into REGNUM.  */


> +  bool frame_related_p = (regnum == SP_REGNUM);

I think it would be better to make the frame-related decision be an
explicit parameter passed to the routine (don't forget SP is not always
the frame pointer).  Then the new uses would pass 'true' and the
existing uses 'false'.

R.