This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] i386: More use reference of struct ix86_frame to avoid copy
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Martin Liška <mliska at suse dot cz>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Uros Bizjak <ubizjak at gmail dot com>, Jan Hubicka <hubicka at ucw dot cz>
- Date: Tue, 16 Jan 2018 08:09:27 -0800
- Subject: Re: [PATCH] i386: More use reference of struct ix86_frame to avoid copy
- Authentication-results: sourceware.org; auth=none
- References: <20180116114035.GA9373@gmail.com> <CAMe9rOohQjFoMxpNYbfWLHYwBWzpuL=B9nBQuw0D_-R+6wsxBQ@mail.gmail.com> <5ffe44bb-9be8-bc27-749c-fa64f9f3fcb4@suse.cz>
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
>>>
>>
>>
>>
>
--
H.J.