[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

[Duncan P. N. Exon Smith]

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.

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