From: Bernd Edlinger <bernd.edlinger@hotmail.de>
To: Daniel Santos <daniel.santos@pobox.com>
Cc: gcc-patches <gcc-patches@gcc.gnu.org>, Uros Bizjak <ubizjak@gmail.com>
Subject: Re: [PATCH] [i386] Recompute the frame layout less often
Date: Mon, 22 May 2017 18:32:00 -0000 [thread overview]
Message-ID: <AM4PR0701MB2162B13BC6E5E1964B927A2BE4F80@AM4PR0701MB2162.eurprd07.prod.outlook.com> (raw)
In-Reply-To: <64f15ee1-da2d-0604-2fb4-8e5fdc435c34@pobox.com>
On 05/19/17 05:17, Daniel Santos wrote:
> On 05/18/2017 08:37 AM, Bernd Edlinger wrote:
>> On 05/17/17 04:01, Daniel Santos wrote:
>>>> - if (ignore_outlined && cfun->machine->call_ms2sysv
>>>> - && in_hard_reg_set_p (stub_managed_regs, DImode, regno))
>>>> - return false;
>>>> + if (ignore_outlined && cfun->machine->call_ms2sysv)
>>>> + {
>>>> + /* Registers who's save & restore will be managed by stubs
>>>> called from
>>>> + pro/epilogue. */
>>>> + HARD_REG_SET stub_managed_regs;
>>>> + xlogue_layout::compute_stub_managed_regs (stub_managed_regs);
>>>>
>>>> + if (in_hard_reg_set_p (stub_managed_regs, DImode, regno))
>>>> + return false;
>>>> + }
>>>> +
>>>> if (crtl->drap_reg
>>>> && regno == REGNO (crtl->drap_reg)
>>>> && !cfun->machine->no_drap_save_restore)
>>> This makes no sense. The entire purpose of stub_managed_regs is to
>>> cache the result of xlogue_layout::compute_stub_managed_regs() and this
>>> would unnecessarily repeat that calculation for each time
>>> ix86_save_reg() is called. Since
>>> xlogue_layout::compute_stub_managed_regs() calls ix86_save_reg many
>>> times, this makes it even worse.Which registers are being saved
>>> out-of-line and inline MUST be known at the time the stack layout is
>>> determined. So stub_managed_regsshould either be left a TU static or
>>> just moved to struct machine_function.
>>>
>>> As an aside, I've noticed that xlogue_layout::compute_stub_managed_regs
>>> is calling ix86_save_reg (which isn't trivial) more often than it really
>>> has to, so I've refactored it.
>>>
>> Well, meanwhile I think the stub_managed_regs contain zero information
>> and need not be saved at all, because it can easily be reconstructed
>> from m->call_ms2sysv_extra_regs.
>>
>> See the attached new version. Daniel does it work for you?
>
> No, I'm not at all comfortable with you making so many seemingly
> unnecessary changes to this code. (Although I wish I got this much
> feedback during my RFCs! :) I can accept the changes to
> is/count_stub_managed_reg (with some caveats), but I do not at all see
> the rationale for changing m_stub_names to a static and adding another
> dimension for the object instance -- from an OO standpoint, that's just
> bad design. Can you please share your rationale for that?
>
Hmm, sorry about that ...
I just thought it would be nice to avoid the const-cast here.
This moved the m_stub_names from all 4 instances to one static
array s_stub_names. But looking at it again, I think the extra
dimension is not even necessary, because all instances share the
same data, so removing that extra dimension again will be fine.
> Incidentally, half of the space in that array is wasted and can be
> trimmed since a given instance of xlogue_layout either supports hard
> frame pointers or doesn't, I just never got around to splitting that
> apart. (The first three enum xlogue_stub values are for without HFP and
> the last three for with.) Also, if we wanted to further reduce the
> memory footprint of xlogue_layout objects, the offset field of struct
> reginfo could be changed to int, and if we really wanted to go nuts then
> 16 bits would work for both of its fields.
>
> So for is/count_stub_managed_reg(s), you are obviously much more
> familiar with gcc, its passes and the i386 backend, but my knowledge
> level makes me very uncomfortable having the result of
> xlogue_layout::is_stub_managed_reg() determined in a way that has the
> (apparent) potential to differ from from the state at the time the last
> call to ix86_compute_frame_layout() was made; I just don't understand
I fund it technically difficult to add a HARD_REG_SET to
struct machine_function, and tried to avoid the extra overhead of
calling ix86_save_reg so often, which you also felt uncomfortable with.
So, if you look at compute_stub_managed_regs I first saw that the
first loop can never terminate thru the "return 0", because the
registers in x86_64_ms_sysv_extra_clobbered_registers are guaranteed
to be clobbered. Then I saw that the bits in stub_managed_regs
are always added in the same sequence, thus the result depends only
on the number call_ms2sysv_extra_regs and hfp so everything is already
available in struct machine_function.
Thanks
Bernd.
next prev parent reply other threads:[~2017-05-22 18:32 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-14 9:11 Bernd Edlinger
2017-05-14 9:58 ` Daniel Santos
2017-05-14 10:27 ` Uros Bizjak
2017-05-14 18:11 ` Bernd Edlinger
2017-05-14 20:37 ` Daniel Santos
2017-05-15 2:23 ` Daniel Santos
2017-05-15 20:41 ` Bernd Edlinger
2017-05-15 23:52 ` Daniel Santos
2017-05-16 5:42 ` Daniel Santos
2017-05-16 8:52 ` Bernd Edlinger
2017-05-16 15:11 ` Daniel Santos
2017-05-16 17:38 ` Ian Lance Taylor via gcc-patches
2017-05-16 20:10 ` Bernd Edlinger
2017-05-16 21:05 ` Bernd Edlinger
2017-05-17 2:12 ` Daniel Santos
2017-05-17 17:43 ` Bernd Edlinger
2017-05-18 7:05 ` Daniel Santos
2017-05-18 13:48 ` Bernd Edlinger
2017-05-19 3:13 ` Daniel Santos
2017-05-22 18:32 ` Bernd Edlinger [this message]
2017-05-23 14:34 ` Bernd Edlinger
2017-05-24 6:49 ` Daniel Santos
2017-06-01 16:06 ` [PING][PATCH] " Bernd Edlinger
2017-06-01 18:18 ` [PATCH] " Uros Bizjak
2017-05-23 23:01 ` Daniel Santos
2017-05-16 21:35 ` Daniel Santos
2017-05-17 18:41 ` Bernd Edlinger
2017-05-18 7:14 ` Daniel Santos
2017-05-19 5:12 ` Daniel Santos
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM4PR0701MB2162B13BC6E5E1964B927A2BE4F80@AM4PR0701MB2162.eurprd07.prod.outlook.com \
--to=bernd.edlinger@hotmail.de \
--cc=daniel.santos@pobox.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).