Re: [PATCH] [i386] Recompute the frame layout less often

[Daniel Santos <daniel dot santos at pobox dot com>]

On 05/22/2017 01:32 PM, Bernd Edlinger wrote:
> On 05/19/17 05:17, Daniel Santos wrote:
>
> > 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.

Well remember const-correctness isn't about an object's internal 
(bitwise) state, but it's externally visible (logical) state.  So a 
const member function need not avoid altering it's internal state if the 
externally visible state remains unchanged, such as when caching some 
result or lazy initing.  I have tended to prefer using const_cast for 
this, isolating its use to a single const accessor function (or if () 
block) to leave less room for the data members to be accidentally 
altered in another const member function.  But mutable is generally 
preferred over const_cast, which opens up the danger of accidentally 
modifying an object's logical state (especially by a subsequent edit), 
so using mutable is probably a better practice anyway.

However, ...

> 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.

You are correct!  And I see that you're new patch has already changed 
get_stub_name to a static member function, so great!

> > 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.

Yes, I agree with how you have refactored compute_stub_managed_regs 
given your rationale of not adding another header dependency to i386.h.  
It is only the overall scheme of calculating this outside of 
ix86_compute_frame_layout that I cannot validate due to my not having a 
good understanding of what can and cannot change in between the time 
that ix86_compute_frame_layout is last called and the last call to 
is_stub_managed_regs().

As Uros said, my patch set touches a "delicate part of the compiler, 
where lots of code-paths cross each other (and we have had quite some 
hard-to-fix bugs in this area)" 
(https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01924.html).  I wrote it 
the way I did with my understanding of what was safe to do and your 
alterations move it's functionality outside of that understanding.  So 
if you say that this won't break it, then I will have to trust you (and 
the testsuite) on that.

On that note, the tests are undergoing some change and bug fixes and I'm 
planning on adding more tests to validate non-breakage with various 
other stack frame-related options and probably additional tests (and 
test options) triggered by GCC_TEST_RUN_EXPENSIVE or some such.

Daniel