[PATCHES] PR18303: Use appropriate Feature flags for encoding instructions

[David Peixotto]

On 1/15/2014 8:57 AM, David Woodhouse wrote:
> On Fri, 2014-01-10 at 15:33 -0800, David Peixotto wrote:
>> I've attached the patch that gets rid of the MCSubtargetInfo data member in
>> the MCCodeEmitter objects. I do this by adding a MCSubtargetInfo parameter
>> to the MCStreamer::EmitInstruction() interface. The subtarget info is then
>> threaded down to the MCCodeEmitter::EncodeInstruction() function.
>>
>> I followed David's lead by storing the MCSubtargetInfo in the
>> MCRelaxableFragment and using it to re-encode the instruction when it is
>> relaxed. This makes the fragment object larger now because it is storing the
>> whole subtarget info object, but it could be made smaller by keeping the
>> subtargets const and uniquing them as David suggested.
>>
>> This change fixes the encoding of relaxable instructions and mode switching
>> in inlineasm. The change was fairly straightforward to make, but it required
>> touching all the backends.
>>
>> Patches are attached in logical steps I took and as one big smashed patch.
>
> This looks good to me (without capitals since I'm not qualified to opine).

I think it moves in a good direction. At least we are explicit now about 
the required sharing of the SubtargetInfo state between the parser and 
encoder.

> I'd like to see this go in, and then perhaps we can follow up afterwards
> with reducing the memory usage by uniquifying the SubtargetInfo and
> storing only a reference in the MCRelaxableFragment...?

As you said the size issue might be a problem. It looks like there is a 
new RelaxableFragment for each instruction that could be relaxed. I 
think the uniqing idea would help, but then that is another potential 
api change (if we make subtargetinfo truly const).

I think another barrier for this patch is getting buy-in for the change 
in the MCStreamer API. I am not sure what kind of approval is needed for 
such a widespread change. Jim, do you have an guidance here?

> Since the first patch is fairly intrusive, it already fails to apply to
> today's HEAD. Here's an updated version...
>
Yes, it is going to quickly become outdated and will need attention 
until it is actually merged. My other question is whether we should 
commit it all at once as a big monolithic change or do it smaller 
bit-by-bit commits.