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

[David Peixotto]

On 1/10/2014 2:15 AM, David Woodhouse wrote:
> On Thu, 2014-01-09 at 14:53 -0800, David Peixotto wrote:
>> On 1/9/2014 3:23 AM, David Woodhouse wrote:
>>> The X86 and ARM TargetAsmParsers are the main users of ToggleFeature().
>>> The way things currently work is the TargetAsmParser and the CodeEmitter
>>> are both initialised with the *same* SubtargetInfo object, so when the
>>> TargetAsmParser frobs it the CodeEmitter sees the change.
>>
>> This is actually not true when parsing inline assembler. For inline
>> assembly a new SubtargetInfo object is created and that leads to more
>> problems.
>
> Perhaps one could argue that it was perfectly true. I was describing the
> way things *work*. You are describing one of the ways in which they
> *don't* work :)

Ok, fair enough :)

> Why do you make a new SubtargetInfo? Is that to deal with the problem
> that was discussed at the end of last year, with inline asm changing the
> mode? How does GCC deal with that issue, if at all?

Yes, it seems that originally a new SubtargetInfo was created to avoid 
problems with the parser changing the mode. I'm coming to the opinion 
that if the programmmer writes these directives in inline assembly then 
we should just respect it and trust that they know what they are doing. 
It seems that GCC has an implicit switch back to the original mode at 
the end of the inlineasm. I would argue this is not necessary and we 
could leave it to the programmers responsibility.

> I still think a '.code prev' directive would be a nicer answer to that,
> and somewhat more compatible (at least in theory) with the concept of an
> external assembler. And the SubtargetInfoFactory that I suggested would
> make that fairly trivial to implement.

Yes I would agree that the programmer should be responsible for 
restoring the mode.

> Now I come to look at the SubtargetInfoFactory idea in a little more
> detail and thinking about how I'd do the uniquification... it looks like
> the feature bits are the only thing that can really change. Things like
> the SchedModel which Jim mentioned also wanting to access are basically
> constant. So the Factory class would only really need to handle the
> feature bits and perhaps it could just keep a single underlying
> SubtargetInfo for everything else.
>
> Making it not a 'Factory' in the normal sense at all. And in fact,
> making me wonder if we wouldn't be better off just storing and passing
> the feature bits themselves as I did in my initial implementation, since
> they're the only thing that's variable.

I'm working on a patch inspired by yours to remove the SubtargetInfo 
from the MCCodeEmitter objects and always pass it directly to the 
EncodeInstruction method. The change touches a lot of places to thread 
the SubtargetInfo to the right place, but the changes are fairly 
mechanical. Passing the SubtargetInfo directly should solve both the 
inlineasm problem and the re-encoding of relaxed instructions.

I will share the patch soon. I'm not sure its the best solution, but 
sometimes it helps to see an actual implementation.