[PATCH] Remove copies of MCSubtargetInfo from MCRelaxableFragment

Duncan P. N. Exon Smith dexonsmith at apple.com
Fri Jul 10 10:17:40 PDT 2015


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

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





More information about the llvm-commits mailing list