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

[Jim Grosbach]

On Jan 8, 2014, at 10:08 AM, David Woodhouse <dwmw2 at infradead.org> wrote:

> On Wed, 2014-01-08 at 09:54 -0800, Jim Grosbach wrote:
>> 
>> Why have this be separate value from what’s in the MCSubtargetInfo?
>> Perhaps naively, but I’d have expected to just add a setter method to
>> that rather than keeping a separate copy of the data, which can go out
>> of sync, in the code emitter itself. If that’s practical, it would
>> potentially simplify a lot of things. For example, the isThumb() and
>> friends methods won’t need a new argument for the feature bits.
> 
> That's kind of the point in the whole thing! :)
> 
> The flags in the MCSubtargetInfo *do* get out of sync. In particular,
> the AsmParser will toggle them as it encounters .code32/.code64
> directives. And the MCCodeEmitter will then immediately output in the
> appropriate mode... yay!
> 
> However, the problems occur when we a *re*-encoding instructions in
> relaxed fragments. We end up encoding them in the mode that the
> AsmParser *happened* to leave the MCSubtargetInfo in, at the end of
> parsing the file.
> 
> Hence storing the *current* feature bits in the MCRelaxableFragment so
> when we later re-encode, we can do it in the right mode.

Right, this all makes complete sense. Keeping the data on the fragment is the right thing to do. Poor phrasing of my question, I think. I don’t follow why the additional information is passed around an an explicit parameter rather than just updating the information in the MCSubtargetInfo via a setter method. For example, something like the following in relaxInstruction():

getEmitter().setSubtargetFeatures(F.getEmitterFeatures());
getEmitter().EncodeInstruction(Relaxed, VecOS, Fixups);

That way the signatures of EncodeInstruction() and all the follow-on helper methods don’t need to change and the logic of those functions stays “STI has the relevant feature bits”.

-Jim