<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 10, 2015 at 10:17 AM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Jul-10, at 06:53, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
> patch 5 looks odd. Why do you need to store a MCSubtargetInfo? Can't<br>
> you store just the feature bits and pass just the feature bits to<br>
> .encodeInstruction?<br>
<br>
I didn't really look into why `encodeInstruction()` needs STI.  Looking,<br>
it's because, e.g.:<br>
<br>
encodeInstruction()<br>
=> getBinaryCodeForInstr()<br>
=> (e.g.) MipsMCCodeEmitter::getMachineOpValue()<br>
=> getExprOpValue()<br>
=> isMicroMips()<br>
=> STI.getFeatureBits()<br>
<br>
The rest of `*::getMachineOpValue()` ignore STI.<br>
<br>
Across the targets it looks like there are 143 different functions being<br>
called from `getBinaryCodeForInstr()`:<br>
--<br>
$ find . -name "*.inc" | xargs grep -l getBinaryCodeForInstr |<br>
  xargs -L1 awk 'found==1 && /STI/ {print} /getBinaryCodeForInstr/ {found=1}' |<br>
  sort -u | sed -e 's,.* = ,,' -e 's,(.*,,' | sort -u |<br>
  wc -l<br>
     143<br>
--<br>
Checking another at random, `NEONThumb2DataIPostEncoder()`, it also just<br>
needs the feature bits (it checks `isThumb2()`).<br>
<br>
So I guess it's probably worth untangling this further.  I could convert<br>
everything `encodeInstruction()` calls transitively to take a<br>
`FeatureBitset` instead of a `const MCSubtargetInfo &`.  Make sense?<br>
<br></blockquote><div><br></div><div>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.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> On 10 July 2015 at 09:42, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>> patch 4 LGTM<br>
>><br>
>> On 10 July 2015 at 09:37, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>>> patch 3 LGTM.<br>
>>><br>
>>> On 10 July 2015 at 09:29, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>>>> Patch 2 LGTM.<br>
>>>><br>
>>>> On 10 July 2015 at 09:27, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>>>>> Patch 1 LGTM.<br>
>>>>><br>
>>>>><br>
>>>>> On 8 July 2015 at 20:30, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>>>>> Last I checked my `-g -flto` workload, `MCRelaxableFragment` was taking<br>
>>>>>> up 4.7% of the heap during LTO.  Since these aren't debug info related,<br>
>>>>>> the relative cost is even higher without `-g` (I figure 10% or so?).<br>
>>>>>> The struct layout contained a shock: over half of<br>
>>>>>> `sizeof(MCRelaxableFragment)` comes from a local copy (!?) of<br>
>>>>>> `MCSubtargetInfo`.<br>
>>>>>><br>
>>>>>> It was more roundabout than I expected to fix, but this patch series<br>
>>>>>> does it.  0001 removes the copy of `MCSchedModel` from<br>
>>>>>> `MCSubtargetInfo`, 0002-0004 change the `MCSubtargetInfo` API to prove<br>
>>>>>> that only the feature bits get mutated, and 0005 moves the copies of<br>
>>>>>> `MCSubtargetInfo` to a cache in `MCContext`.<br>
>>>>>><br>
>>>>>> I'd love a pre-commit sanity check from someone for two of these:<br>
>>>>>>  - 0001 (the object for `MCSchedModel::GetDefaultMCSubtarget()` doesn't<br>
>>>>>>    need to be defined in the header for some reason, does it?) and<br>
>>>>>>  - 0003 (I'm touching *all* the targets...)<br>
>>>>>><br>
>>>>>> and I've provided the rest of the series (and a combined all.patch) for<br>
>>>>>> context.<br>
>>>>>><br>
>>>>>><br>
<br>
</blockquote></div></div>