[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

[Duncan P. N. Exon Smith]

> On 2015-Jul-12, at 16:23, Eric Christopher <echristo at gmail.com> wrote:
> 
> 
> 
> On Sun, Jul 12, 2015 at 4:11 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> I didn't measure 0005 alone, but the full patch series together
> saved something around (IIRC just over) 2%.  On a non-debug info
> workload, I imagine it would be much more than that.  Some of
> the savings (how much?) are already there, since 0001 made
> MCSubtargetInfo a little smaller.
> 
> I don't think there is a central MCSubtargetInfo available.
> 
> It *is* wasteful to have the FeatureBitset as well as the
> MCSubtargetInfo.  Eric and I chatted a little on Friday, and the
> other approach would be to make MCSubtargetInfo const in the
> AsmParser (since STI is immutable, we could drop the
> FeaturesBitset from MCRelaxableFragment).  Instead of calling
> ToggleFeature/etc. all over, AsmParser would request a different
> MCSubtargetInfo from the cache in MCContext.
> 
> What do you think about that direction?  I'm not volunteering to
> do it (right now), but I could document it in MCRelaxableFragment
> when I commit.
> 
> 
> To be clear: I'm against committing the patch as is.

but...

> > On 10 July 2015 at 20:45, Eric Christopher <echristo at gmail.com> wrote:
> >>
> >>>
> >>>> You could, but unless there's a compelling reason I'd rather not. It
> >>>> seems like we'd be passing an implementation detail around rather than
> >>>> something that gives us access to what we want.
> >>>>
> >>>
> >>> Eric, do you prefer the STI cache in patch 0005?
> >>
> >>
> >> I do, yes. At least as a temporary workaround.

?

> 
> Yes, it'll be more work to fix our layering, hierarchy etc such that what's needed in certain cases is there without work, but this just makes the existing code more confusing for a small win.

(IMO, not small.)

> You've now pulled out the various bits of a data structure, copied them, stuck them in an object... alongside an object that should contain the same bits. No.
> 
> The cache idea seems to be a reasonably clean workaround without just adding hacks.

But might require rewriting the parser, which I don't have time
for, and if it ends up making the assembler slower could spiral
into a fairly big project.  I'll have a look tomorrow, but I may
just drop this for now.

(IMO, 0005 is less gross than a copy of MCSubtargetInfo on every
MCRelaxableFragment.)

>  
> > On 2015-Jul-11, at 05:10, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> >
> > Duncan, what is the memory savings of the original patch? You still
> > store a MCSubtargetInfo reference and a the bits. Can't you at least
> > drop the MCSubtargetInfo and reconstruct it just before calling
> > encodeInstruction with some "central" MCSubtargetInfo?
> >
> > On 10 July 2015 at 20:45, Eric Christopher <echristo at gmail.com> wrote:
> >>
> >>>
> >>>> You could, but unless there's a compelling reason I'd rather not. It
> >>>> seems like we'd be passing an implementation detail around rather than
> >>>> something that gives us access to what we want.
> >>>>
> >>>
> >>> Eric, do you prefer the STI cache in patch 0005?
> >>
> >>
> >> I do, yes. At least as a temporary workaround.
> >>
> >>>
> >>>
> >>> I agree with Rafael that it's pretty ugly if it's not actually
> >>> necessary (although I haven't confirmed that every callsite is okay
> >>> with only the feature bits, it seems likely).
> >>>
> >>> Is `FeatureBitset` really that much of an implementation detail?
> >>> Every `MCSubtargetInfo` has one in its public API...
> >>>
> >>
> >> I mean, we could pass around FeatureBitset instead of TargetSubtargetInfo
> >> through the backend as well. Save a lot of space.
> >>
> >> I jest, a little, but it's the same thing (quite literally in a lot of
> >> cases). Just because our interfaces are pretty crappy doesn't mean we should
> >> just avoid them.
> >>
> >> -eric
> >>
> >>>
> >>>
> >>>> -eric
> >>>>
> >>>>>
> >>>>> On 10 July 2015 at 09:42, Rafael Espíndola
> >>>>> <rafael.espindola at gmail.com> wrote:
> >>>>>> patch 4 LGTM
> >>>>>>
> >>>>>> On 10 July 2015 at 09:37, Rafael Espíndola
> >>>>>> <rafael.espindola at gmail.com> wrote:
> >>>>>>> patch 3 LGTM.
> >>>>>>>
> >>>>>>> On 10 July 2015 at 09:29, Rafael Espíndola
> >>>>>>> <rafael.espindola at gmail.com> wrote:
> >>>>>>>> Patch 2 LGTM.
> >>>>>>>>
> >>>>>>>> On 10 July 2015 at 09:27, Rafael Espíndola
> >>>>>>>> <rafael.espindola at gmail.com> wrote:
> >>>>>>>>> Patch 1 LGTM.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 8 July 2015 at 20:30, Duncan P. N. Exon Smith
> >>>>>>>>> <dexonsmith at apple.com> wrote:
> >>>>>>>>>> Last I checked my `-g -flto` workload, `MCRelaxableFragment` was
> >>>>>>>>>> taking
> >>>>>>>>>> up 4.7% of the heap during LTO.  Since these aren't debug info
> >>>>>>>>>> related,
> >>>>>>>>>> the relative cost is even higher without `-g` (I figure 10% or
> >>>>>>>>>> so?).
> >>>>>>>>>> The struct layout contained a shock: over half of
> >>>>>>>>>> `sizeof(MCRelaxableFragment)` comes from a local copy (!?) of
> >>>>>>>>>> `MCSubtargetInfo`.
> >>>>>>>>>>
> >>>>>>>>>> It was more roundabout than I expected to fix, but this patch
> >>>>>>>>>> series
> >>>>>>>>>> does it.  0001 removes the copy of `MCSchedModel` from
> >>>>>>>>>> `MCSubtargetInfo`, 0002-0004 change the `MCSubtargetInfo` API to
> >>>>>>>>>> prove
> >>>>>>>>>> that only the feature bits get mutated, and 0005 moves the copies
> >>>>>>>>>> of
> >>>>>>>>>> `MCSubtargetInfo` to a cache in `MCContext`.
> >>>>>>>>>>
> >>>>>>>>>> I'd love a pre-commit sanity check from someone for two of these:
> >>>>>>>>>> - 0001 (the object for `MCSchedModel::GetDefaultMCSubtarget()`
> >>>>>>>>>> doesn't
> >>>>>>>>>>   need to be defined in the header for some reason, does it?)
> >>>>>>>>>> and
> >>>>>>>>>> - 0003 (I'm touching *all* the targets...)
> >>>>>>>>>>
> >>>>>>>>>> and I've provided the rest of the series (and a combined
> >>>>>>>>>> all.patch) for
> >>>>>>>>>> context.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>
> >>>
> >>