[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

[Duncan P. N. Exon Smith]

> On 2015-Jul-10, at 10:21, Eric Christopher <echristo at gmail.com> wrote:
> 
> 
> 
> On Fri, Jul 10, 2015 at 10:17 AM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015-Jul-10, at 06:53, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> >
> > patch 5 looks odd. Why do you need to store a MCSubtargetInfo? Can't
> > you store just the feature bits and pass just the feature bits to
> > .encodeInstruction?
> 
> I didn't really look into why `encodeInstruction()` needs STI.  Looking,
> it's because, e.g.:
> 
> encodeInstruction()
> => getBinaryCodeForInstr()
> => (e.g.) MipsMCCodeEmitter::getMachineOpValue()
> => getExprOpValue()
> => isMicroMips()
> => STI.getFeatureBits()
> 
> The rest of `*::getMachineOpValue()` ignore STI.
> 
> Across the targets it looks like there are 143 different functions being
> called from `getBinaryCodeForInstr()`:
> --
> $ find . -name "*.inc" | xargs grep -l getBinaryCodeForInstr |
>   xargs -L1 awk 'found==1 && /STI/ {print} /getBinaryCodeForInstr/ {found=1}' |
>   sort -u | sed -e 's,.* = ,,' -e 's,(.*,,' | sort -u |
>   wc -l
>      143
> --
> Checking another at random, `NEONThumb2DataIPostEncoder()`, it also just
> needs the feature bits (it checks `isThumb2()`).
> 
> So I guess it's probably worth untangling this further.  I could convert
> everything `encodeInstruction()` calls transitively to take a
> `FeatureBitset` instead of a `const MCSubtargetInfo &`.  Make sense?
> 
> 
> 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 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...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-MC-Remove-the-copy-of-STI-in-MCRelaxableFragment.patch
Type: application/octet-stream
Size: 8990 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150710/f0de6f37/attachment.obj>
-------------- next part --------------

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