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

[David Woodhouse]

On Wed, 2014-01-08 at 13:34 -0800, Jim Grosbach wrote:
> In theory what we really want is to have the MCSubtargetInfo instance
> itself be ‘const’ but allow creating and passing around more than one
> of them.

That has deeper ramifications. If you do that, you *also* want to do it
consistently. So *all* MCSubtargetInfo instances should be 'const'. 

And in that case, you'll want to kill the ToggleFeature() method on it,
and the resetSubtargetFeatures() method too.

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. It's somewhat
non-trivial for the TargetAsmParser to just pass a *new* STI to the
CodeEmitter, as you glibly suggest :)

How about this... kill the ToggleFeature() and resetSubtargetFeatures()
methods on the MCSubtargetInfo class, and it is *always* const.¹

Introduce a new subclass 'MCSubtargetInfoFactory' which does have the
ToggleFeature() and resetSubtargetFeatures() methods¹. It will
internally keep a *set* of const MCSubtargetInfo objects, and create a
new one when it needs to (doing the uniquification for itself).

Then add a getCurrentSubtargetInfo() method to the factory class too, to
get the *current* SubtargetInfo for storing in the MCRelaxableFragment
or wherever else you want it, and passing to the CodeEmitter's
EncodeInstruction() method.

In the first instance, the CodeEmitter would still have the
MCSubtargetInfoFactory in place of the current MCSubtargetInfo, and
would still use that when no explicit STI is passed to
EncodeInstruction. So we still have the inconsistency there that you
objected to. But what I describe above is the path to *fixing* that
inconsistency — we could then look at removing the STI from the
CodeEmitters and *always* passing it in from the caller of
EncodeInstruction, having stored it... elsewhere.

On the whole, I think the full scope of this is probably beyond me at
this point. Especially as I just received a shipping notice for a new
platform which will arrive in a few days and I'm actually going to need
to do my day job for a while.

I am inclined to suggest that we go with what I've posted so far, which
you said was a step in the right direction and does at least fix the
issue at hand. And then, as you said, it's simple enough to replace the
existing 'Features' argument that I've threaded through the CodeEmitters
with a 'STI' argument. That relatively simple search/replace will be a
whole lot easier than adding it to all the EncoderMethod and
PostEncoderMethod functions in all the back ends in the first place,
which I've already done (and which won't apply cleanly for long, I'm
sure).

-- 
dwmw2

¹ Changing only the names of the classes, you could alternatively
  describe this as: introduce a new MCConstSubtargetInfo parent class
  of MCSubtargetInfo, which lacks the non-const methods. Then add
  the 'factory' capabilities and get getCurrentConstSubtargetInfo()
  method to the MCSubtargetInfo class.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140109/574da362/attachment.bin>