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

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

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?

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 
well enough what all can change in between the last call to 
ix86_compute_frame_layout() and the last call to 
xlogue_layout::is_stub_managed_reg().  I like your 
count_stub_managed_regs() is_stub_managed_regs() from a *performance* 
standpoint (and I know I get too uptight about that kind of thing, so 
appreciate that), but as to the change in scheme, I would have to trust 
you if you assert that this will always behave consistently.

I also want to give you a little background on some of these seemingly 
repetitive computations.  One of my design goals was for the code to be 
relatively easily to adapted to the management of out-of-line 
pro/epilogue stubs for other possible scenarios where there are a lot of 
clobbers and it could be useful.  Granted, I don't have enough knowledge 
of x86 architectures to identify situations other than this one (in 
64-bit Wine) where it could be helpful and I know that x86 push/pops are 
really small.  So theoretically, struct machine_function's 
"call_ms2sysv" could be changed to something like "outline_savres" and 
any combination of clobbered registers for which there is a descent stub 
could be used if it was a good choice.  I also realize that nobody likes 
complexity that isn't being used, and I respect that.  So if you are 
comfortable with this change and you believe you understand how it works 
then I will agree to it, but I'll be trusting you well beyond my 
knowledge level and ability to confidently predict the outcome (probably 
what a programmer hates the most).

Thanks,
Daniel