<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sun, Jul 12, 2015 at 4:11 PM 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">I didn't measure 0005 alone, but the full patch series together<br>
saved something around (IIRC just over) 2%.  On a non-debug info<br>
workload, I imagine it would be much more than that.  Some of<br>
the savings (how much?) are already there, since 0001 made<br>
MCSubtargetInfo a little smaller.<br>
<br>
I don't think there is a central MCSubtargetInfo available.<br>
<br>
It *is* wasteful to have the FeatureBitset as well as the<br>
MCSubtargetInfo.  Eric and I chatted a little on Friday, and the<br>
other approach would be to make MCSubtargetInfo const in the<br>
AsmParser (since STI is immutable, we could drop the<br>
FeaturesBitset from MCRelaxableFragment).  Instead of calling<br>
ToggleFeature/etc. all over, AsmParser would request a different<br>
MCSubtargetInfo from the cache in MCContext.<br>
<br>
What do you think about that direction?  I'm not volunteering to<br>
do it (right now), but I could document it in MCRelaxableFragment<br>
when I commit.<br>
<br></blockquote><div><br></div><div>To be clear: I'm against committing the patch as is.</div><div><br></div><div>Yes, it'll be more work to fix our layering, hierarchy etc such that what's needed in certain cases is there without work, but this just makes the existing code more confusing for a small win. You've now pulled out the various bits of a data structure, copied them, stuck them in an object... alongside an object that should contain the same bits. No.</div><div><br></div><div>The cache idea seems to be a reasonably clean workaround without just adding hacks.</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">
> On 2015-Jul-11, at 05:10, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
> Duncan, what is the memory savings of the original patch? You still<br>
> store a MCSubtargetInfo reference and a the bits. Can't you at least<br>
> drop the MCSubtargetInfo and reconstruct it just before calling<br>
> encodeInstruction with some "central" MCSubtargetInfo?<br>
><br>
> On 10 July 2015 at 20:45, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
>><br>
>>><br>
>>>> You could, but unless there's a compelling reason I'd rather not. It<br>
>>>> seems like we'd be passing an implementation detail around rather than<br>
>>>> something that gives us access to what we want.<br>
>>>><br>
>>><br>
>>> Eric, do you prefer the STI cache in patch 0005?<br>
>><br>
>><br>
>> I do, yes. At least as a temporary workaround.<br>
>><br>
>>><br>
>>><br>
>>> I agree with Rafael that it's pretty ugly if it's not actually<br>
>>> necessary (although I haven't confirmed that every callsite is okay<br>
>>> with only the feature bits, it seems likely).<br>
>>><br>
>>> Is `FeatureBitset` really that much of an implementation detail?<br>
>>> Every `MCSubtargetInfo` has one in its public API...<br>
>>><br>
>><br>
>> I mean, we could pass around FeatureBitset instead of TargetSubtargetInfo<br>
>> through the backend as well. Save a lot of space.<br>
>><br>
>> I jest, a little, but it's the same thing (quite literally in a lot of<br>
>> cases). Just because our interfaces are pretty crappy doesn't mean we should<br>
>> just avoid them.<br>
>><br>
>> -eric<br>
>><br>
>>><br>
>>><br>
>>>> -eric<br>
>>>><br>
>>>>><br>
>>>>> On 10 July 2015 at 09:42, Rafael Espíndola<br>
>>>>> <<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<br>
>>>>>> <<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<br>
>>>>>>> <<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<br>
>>>>>>>> <<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<br>
>>>>>>>>> <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>>>>>>>>>> Last I checked my `-g -flto` workload, `MCRelaxableFragment` was<br>
>>>>>>>>>> taking<br>
>>>>>>>>>> up 4.7% of the heap during LTO.  Since these aren't debug info<br>
>>>>>>>>>> related,<br>
>>>>>>>>>> the relative cost is even higher without `-g` (I figure 10% or<br>
>>>>>>>>>> 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<br>
>>>>>>>>>> series<br>
>>>>>>>>>> does it.  0001 removes the copy of `MCSchedModel` from<br>
>>>>>>>>>> `MCSubtargetInfo`, 0002-0004 change the `MCSubtargetInfo` API to<br>
>>>>>>>>>> prove<br>
>>>>>>>>>> that only the feature bits get mutated, and 0005 moves the copies<br>
>>>>>>>>>> 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()`<br>
>>>>>>>>>> doesn't<br>
>>>>>>>>>>   need to be defined in the header for some reason, does it?)<br>
>>>>>>>>>> and<br>
>>>>>>>>>> - 0003 (I'm touching *all* the targets...)<br>
>>>>>>>>>><br>
>>>>>>>>>> and I've provided the rest of the series (and a combined<br>
>>>>>>>>>> all.patch) for<br>
>>>>>>>>>> context.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>><br>
>>><br>
>><br>
<br>
</blockquote></div></div>