Re: [PATCH] xtensa: Optimize stack frame adjustment more

[Takayuki 'January June' Suwa <jjsuwa_sys3175@yahoo.co.jp>]

On 2023/01/06 6:32, Max Filippov wrote:
> Hi Suwa-san,
Hi!

> 
> On Thu, Jan 5, 2023 at 3:57 AM Takayuki 'January June' Suwa
> <jjsuwa_sys3175@yahoo.co.jp> wrote:
>>
>> This patch introduces a convenient helper function for integer immediate
>> addition with scratch register as needed, that splits and emits either
>> up to two ADDI/ADDMI machine instructions or an addition by register
>> following an immediate integer load (which may later be transformed by
>> constantsynth).
>>
>> By using the helper function, it makes stack frame adjustment logic
>> simplified and instruction count less in some cases.
>>
>> gcc/ChangeLog:
>>
>>         * config/xtensa/xtensa.cc
>>         (xtensa_split_imm_two_addends, xtensa_emit_add_imm):
>>         New helper functions.
>>         (xtensa_emit_adjust_stack_ptr, xtensa_set_return_address,
>>         xtensa_output_mi_thunk): Change to use the helper function.
>> ---
>>  gcc/config/xtensa/xtensa.cc | 139 +++++++++++++++++++++++-------------
>>  1 file changed, 88 insertions(+), 51 deletions(-)
> 
> This change introduces a bunch of failures in the g++ testsuite,
> but the culprit is apparently somewhere in the libstdc++.so, I'm
> still looking for it.
> 
> I see the following pattern change in the generated epilogue code:
> 
> -    4aaf:      b0a192          movi    a9, 0x1b0
> -    4ab2:      1f9a            add.n   a1, a15, a9
> ...
> -    4abe:      20c112          addi    a1, a1, 32
> -    4ac1:      f00d            ret.n
> +    4aaf:      02df12          addmi   a1, a15, 0x200
> +    4ab2:      b0c112          addi    a1, a1, -80
> ...
> +    4abf:      20c112          addi    a1, a1, 32
> +    4ac2:      f00d            ret.n
> 
> I.e. a1 is first moved into the parent stack frame, then back to the right
> spot. This does not look correct, especially for bare-metal targets.
> 

On second thought, it cannot be a good idea to split addition/subtraction to the stack pointer.

> -    4aaf:      b0a192          movi    a9, 0x1b0
> -    4ab2:      1f9a            add.n   a1, a15, a9

> +    4aaf:      02df12          addmi   a1, a15, 0x200
> +    4ab2:      b0c112          addi    a1, a1, -80

Because the former is atomic, but the latter is not. (may be interrupted between the two add instructions)

I'll wait for the results of your investigation, but it may be better to withdraw the patch.