i386: More use reference of struct ix86_frame to avoid copy

This patch has been used with my Spectre backport for GCC 7 for many
weeks and has been checked into GCC 7 branch.  Should I revert it on
GCC 7 branch or check it into trunk?

H.J.
---
When there is no need to make a copy of ix86_frame, we can use reference
of struct ix86_frame to avoid copy.

	* config/i386/i386.c (ix86_expand_prologue): Use reference of
	struct ix86_frame.
	(ix86_expand_epilogue): Likewise.
---
 gcc/config/i386/i386.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

On Tue, Jan 16, 2018 at 3:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> This patch has been used with my Spectre backport for GCC 7 for many
> weeks and has been checked into GCC 7 branch.  Should I revert it on
> GCC 7 branch or check it into trunk?

Ada build failed with this on trunk:

raised STORAGE_ERROR : stack overflow or erroneous memory access
make[5]: *** [/export/gnu/import/git/sources/gcc/gcc/ada/Make-generated.in:45:
ada/sinfo.h] Error 1

Let me revert it on gcc-7-branch.

H.J.
> H.J.
> ---
> When there is no need to make a copy of ix86_frame, we can use reference
> of struct ix86_frame to avoid copy.
>
>         * config/i386/i386.c (ix86_expand_prologue): Use reference of
>         struct ix86_frame.
>         (ix86_expand_epilogue): Likewise.
> ---
>  gcc/config/i386/i386.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index bfb31db8752..9eba3ffd5d6 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -13385,7 +13385,6 @@ ix86_expand_prologue (void)
>  {
>    struct machine_function *m = cfun->machine;
>    rtx insn, t;
> -  struct ix86_frame frame;
>    HOST_WIDE_INT allocate;
>    bool int_registers_saved;
>    bool sse_registers_saved;
> @@ -13413,7 +13412,7 @@ ix86_expand_prologue (void)
>    m->fs.sp_valid = true;
>    m->fs.sp_realigned = false;
>
> -  frame = m->frame;
> +  struct ix86_frame &frame = cfun->machine->frame;
>
>    if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl))
>      {
> @@ -14291,7 +14290,6 @@ ix86_expand_epilogue (int style)
>  {
>    struct machine_function *m = cfun->machine;
>    struct machine_frame_state frame_state_save = m->fs;
> -  struct ix86_frame frame;
>    bool restore_regs_via_mov;
>    bool using_drap;
>    bool restore_stub_is_tail = false;
> @@ -14304,7 +14302,7 @@ ix86_expand_epilogue (int style)
>      }
>
>    ix86_finalize_stack_frame_flags ();
> -  frame = m->frame;
> +  struct ix86_frame &frame = cfun->machine->frame;
>
>    m->fs.sp_realigned = stack_realign_fp;
>    m->fs.sp_valid = stack_realign_fp
> --
> 2.14.3
>

On 01/16/2018 01:35 PM, H.J. Lu wrote:
> On Tue, Jan 16, 2018 at 3:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> This patch has been used with my Spectre backport for GCC 7 for many
>> weeks and has been checked into GCC 7 branch.  Should I revert it on
>> GCC 7 branch or check it into trunk?
> 
> Ada build failed with this on trunk:
> 
> raised STORAGE_ERROR : stack overflow or erroneous memory access
> make[5]: *** [/export/gnu/import/git/sources/gcc/gcc/ada/Make-generated.in:45:
> ada/sinfo.h] Error 1

Hello.

I know that you've already reverted the change, but it's possible to replace
struct ix86_frame &frame = cfun->machine->frame;

with:
struct ix86_frame *frame = &cfun->machine->frame;

And replace usages with point access operator (->). That would also avoid copying.

One another question. After you switched to references, isn't the behavior of function
ix86_expand_epilogue as it also contains write to frame struct like:

 14799    /* Special care must be taken for the normal return case of a function
 14800       using eh_return: the eax and edx registers are marked as saved, but
 14801       not restored along this path.  Adjust the save location to match.  */
 14802    if (crtl->calls_eh_return && style != 2)
 14803      frame.reg_save_offset -= 2 * UNITS_PER_WORD;

Thanks for clarification.
Martin

> 
> Let me revert it on gcc-7-branch.
> 
> H.J.
>> H.J.
>> ---
>> When there is no need to make a copy of ix86_frame, we can use reference
>> of struct ix86_frame to avoid copy.
>>
>>         * config/i386/i386.c (ix86_expand_prologue): Use reference of
>>         struct ix86_frame.
>>         (ix86_expand_epilogue): Likewise.
>> ---
>>  gcc/config/i386/i386.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index bfb31db8752..9eba3ffd5d6 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -13385,7 +13385,6 @@ ix86_expand_prologue (void)
>>  {
>>    struct machine_function *m = cfun->machine;
>>    rtx insn, t;
>> -  struct ix86_frame frame;
>>    HOST_WIDE_INT allocate;
>>    bool int_registers_saved;
>>    bool sse_registers_saved;
>> @@ -13413,7 +13412,7 @@ ix86_expand_prologue (void)
>>    m->fs.sp_valid = true;
>>    m->fs.sp_realigned = false;
>>
>> -  frame = m->frame;
>> +  struct ix86_frame &frame = cfun->machine->frame;
>>
>>    if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl))
>>      {
>> @@ -14291,7 +14290,6 @@ ix86_expand_epilogue (int style)
>>  {
>>    struct machine_function *m = cfun->machine;
>>    struct machine_frame_state frame_state_save = m->fs;
>> -  struct ix86_frame frame;
>>    bool restore_regs_via_mov;
>>    bool using_drap;
>>    bool restore_stub_is_tail = false;
>> @@ -14304,7 +14302,7 @@ ix86_expand_epilogue (int style)
>>      }
>>
>>    ix86_finalize_stack_frame_flags ();
>> -  frame = m->frame;
>> +  struct ix86_frame &frame = cfun->machine->frame;
>>
>>    m->fs.sp_realigned = stack_realign_fp;
>>    m->fs.sp_valid = stack_realign_fp
>> --
>> 2.14.3
>>
> 
> 
>

On Tue, Jan 16, 2018 at 7:03 AM, Martin Liška <mliska@suse.cz> wrote:
> On 01/16/2018 01:35 PM, H.J. Lu wrote:
>> On Tue, Jan 16, 2018 at 3:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> This patch has been used with my Spectre backport for GCC 7 for many
>>> weeks and has been checked into GCC 7 branch.  Should I revert it on
>>> GCC 7 branch or check it into trunk?
>>
>> Ada build failed with this on trunk:
>>
>> raised STORAGE_ERROR : stack overflow or erroneous memory access
>> make[5]: *** [/export/gnu/import/git/sources/gcc/gcc/ada/Make-generated.in:45:
>> ada/sinfo.h] Error 1
>
> Hello.
>
> I know that you've already reverted the change, but it's possible to replace
> struct ix86_frame &frame = cfun->machine->frame;
>
> with:
> struct ix86_frame *frame = &cfun->machine->frame;
>
> And replace usages with point access operator (->). That would also avoid copying.

Won't it be equivalent to reference?

> One another question. After you switched to references, isn't the behavior of function
> ix86_expand_epilogue as it also contains write to frame struct like:
>
>  14799    /* Special care must be taken for the normal return case of a function
>  14800       using eh_return: the eax and edx registers are marked as saved, but
>  14801       not restored along this path.  Adjust the save location to match.  */
>  14802    if (crtl->calls_eh_return && style != 2)
>  14803      frame.reg_save_offset -= 2 * UNITS_PER_WORD;

That could be the issue.  I will double check it.

Thanks.

H.J.
> Thanks for clarification.
> Martin
>
>>
>> Let me revert it on gcc-7-branch.
>>
>> H.J.
>>> H.J.
>>> ---
>>> When there is no need to make a copy of ix86_frame, we can use reference
>>> of struct ix86_frame to avoid copy.
>>>
>>>         * config/i386/i386.c (ix86_expand_prologue): Use reference of
>>>         struct ix86_frame.
>>>         (ix86_expand_epilogue): Likewise.
>>> ---
>>>  gcc/config/i386/i386.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>>> index bfb31db8752..9eba3ffd5d6 100644
>>> --- a/gcc/config/i386/i386.c
>>> +++ b/gcc/config/i386/i386.c
>>> @@ -13385,7 +13385,6 @@ ix86_expand_prologue (void)
>>>  {
>>>    struct machine_function *m = cfun->machine;
>>>    rtx insn, t;
>>> -  struct ix86_frame frame;
>>>    HOST_WIDE_INT allocate;
>>>    bool int_registers_saved;
>>>    bool sse_registers_saved;
>>> @@ -13413,7 +13412,7 @@ ix86_expand_prologue (void)
>>>    m->fs.sp_valid = true;
>>>    m->fs.sp_realigned = false;
>>>
>>> -  frame = m->frame;
>>> +  struct ix86_frame &frame = cfun->machine->frame;
>>>
>>>    if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl))
>>>      {
>>> @@ -14291,7 +14290,6 @@ ix86_expand_epilogue (int style)
>>>  {
>>>    struct machine_function *m = cfun->machine;
>>>    struct machine_frame_state frame_state_save = m->fs;
>>> -  struct ix86_frame frame;
>>>    bool restore_regs_via_mov;
>>>    bool using_drap;
>>>    bool restore_stub_is_tail = false;
>>> @@ -14304,7 +14302,7 @@ ix86_expand_epilogue (int style)
>>>      }
>>>
>>>    ix86_finalize_stack_frame_flags ();
>>> -  frame = m->frame;
>>> +  struct ix86_frame &frame = cfun->machine->frame;
>>>
>>>    m->fs.sp_realigned = stack_realign_fp;
>>>    m->fs.sp_valid = stack_realign_fp
>>> --
>>> 2.14.3
>>>
>>
>>
>>
>

On Tue, Jan 16, 2018 at 8:09 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jan 16, 2018 at 7:03 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 01/16/2018 01:35 PM, H.J. Lu wrote:
>>> On Tue, Jan 16, 2018 at 3:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> This patch has been used with my Spectre backport for GCC 7 for many
>>>> weeks and has been checked into GCC 7 branch.  Should I revert it on
>>>> GCC 7 branch or check it into trunk?
>>>
>>> Ada build failed with this on trunk:
>>>
>>> raised STORAGE_ERROR : stack overflow or erroneous memory access
>>> make[5]: *** [/export/gnu/import/git/sources/gcc/gcc/ada/Make-generated.in:45:
>>> ada/sinfo.h] Error 1
>>
>> Hello.
>>
>> I know that you've already reverted the change, but it's possible to replace
>> struct ix86_frame &frame = cfun->machine->frame;
>>
>> with:
>> struct ix86_frame *frame = &cfun->machine->frame;
>>
>> And replace usages with point access operator (->). That would also avoid copying.
>
> Won't it be equivalent to reference?
>
>> One another question. After you switched to references, isn't the behavior of function
>> ix86_expand_epilogue as it also contains write to frame struct like:
>>
>>  14799    /* Special care must be taken for the normal return case of a function
>>  14800       using eh_return: the eax and edx registers are marked as saved, but
>>  14801       not restored along this path.  Adjust the save location to match.  */
>>  14802    if (crtl->calls_eh_return && style != 2)
>>  14803      frame.reg_save_offset -= 2 * UNITS_PER_WORD;
>
> That could be the issue.  I will double check it.
>

Revert the ix86_expand_epilogue change fixes the ada build.  I opened:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83905