[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

[Eric Christopher]

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.

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

-eric


> > 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.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>
> >>>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150712/54825958/attachment.html>