[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

[Eric Christopher]

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


> >
> > 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/20150710/56a189fb/attachment.html>