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

[Jim Grosbach]

On Jan 8, 2014, at 1:11 PM, David Woodhouse <dwmw2 at infradead.org> wrote:

> On Wed, 2014-01-08 at 10:59 -0800, Jim Grosbach wrote:
>> 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.
> 
> That could perhaps have been easier. However, I'm not sure I like it
> very much.
> 
> The fundamental problem here is that we have *state* in the code
> emitter, which is being prodded from elsewhere (yay, I love the way the
> AsmParser toggles the bits in the MCSubtargetInfo which affects the
> output of the MCCodeEmitter which is what got us PR18303 in the first
> place).
> 
> Yes, I can do a nice simple fix as shown below, but then I find myself
> wondering if I've stomped on the MCSubtargetInfo state while someone
> else might still be relying on it *not* changing. I suppose it makes
> sense that the relaxation will only happen after *all* the initial code
> emission and the mode changes from the input have been handled. But I'd
> have to *prove* that, and declare that it will remain true in
> perpetuity. And even then it's just icky.
> 
> It might be simpler, and it might work, but I just don't *like* it.

The assertion currently in the code that the MCSubtargetInfo is const is, simply, just false for an assembler. It seems like it wouldn’t be (a CPU doesn’t get new ISA extensions partway through, after all), but assemblers can do crazy things like switch which CPU is being targeted in the middle of a file (and worse).

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. When something like a .code directive switches something that contradicts the current instance, create a new one. Which instance was in effect for which bits of stuff would be passed along as an attribute of the fragment, much like what you’re doing here for the relaxation fragments, but more generalized. The instances could be uniqued via the MCContext or something, so we wouldn’t be creating huge numbers of the things.

The current direction makes the simplifying assumption that what actually changes is only the feature bits, so instead of passing around whole new MCSubtargetInfo objects, we just keep a single one and make it mutable. That’ll work out for now, but will break down when we want to generalize. That said, it moves in the right direction and won’t actively interfere with the more general solution later, which would just be switching out the EmitterFeatures ivar with an MCSubtargetInfo& and threading that through.


> But I will defer to your opinion.
> 
> 
> 
> Oh, and it doesn't work anyway:
> 
> X86MCCodeEmitter.cpp:49:9: error: no matching member function for call to
>      'ToggleFeature'
>    STI.ToggleFeature(STI.getFeatureBits() ^ Features);
>    ~~~~^~~~~~~~~~~~~
> /home/dwmw2/llvm/include/llvm/MC/MCSubtargetInfo.h:80:12: note: candidate
>      function not viable: 'this' argument has type 'const
>      llvm::MCSubtargetInfo', but method is not marked const
>  uint64_t ToggleFeature(uint64_t FB);
> 
> 
> That could potentially be fixed... although it's somewhat non-trivial too.

Yeah, things will need de-constified. Blech.

> diff --git a/include/llvm/MC/MCAssembler.h b/include/llvm/MC/MCAssembler.h
> index 8735a55..44a6f87 100644
> --- a/include/llvm/MC/MCAssembler.h
> +++ b/include/llvm/MC/MCAssembler.h
> @@ -294,6 +294,9 @@ class MCRelaxableFragment : public MCEncodedFragmentWithFixups {
>   /// Fixups - The list of fixups in this fragment.
>   SmallVector<MCFixup, 1> Fixups;
> 
> +  /// EmitterFeatures - The feature set for MCCodeEmitter to use.
> +  uint64_t EmitterFeatures;
> +
> public:
>   MCRelaxableFragment(const MCInst &_Inst, MCSectionData *SD = 0)
>     : MCEncodedFragmentWithFixups(FT_Relaxable, SD), Inst(_Inst) {
> @@ -305,6 +308,9 @@ public:
>   const MCInst &getInst() const { return Inst; }
>   void setInst(const MCInst& Value) { Inst = Value; }
> 
> +  uint64_t getEmitterFeatures() const { return EmitterFeatures; }
> +  void setEmitterFeatures(uint64_t features) { EmitterFeatures = features; }
> +
>   SmallVectorImpl<MCFixup> &getFixups() {
>     return Fixups;
>   }
> diff --git a/include/llvm/MC/MCCodeEmitter.h b/include/llvm/MC/MCCodeEmitter.h
> index 9bfa08e..fd5af2c 100644
> --- a/include/llvm/MC/MCCodeEmitter.h
> +++ b/include/llvm/MC/MCCodeEmitter.h
> @@ -11,6 +11,7 @@
> #define LLVM_MC_MCCODEEMITTER_H
> 
> #include "llvm/Support/Compiler.h"
> +#include "llvm/Support/DataTypes.h"
> 
> namespace llvm {
> class MCFixup;
> @@ -36,6 +37,14 @@ public:
>   /// stream \p OS.
>   virtual void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,
>                                  SmallVectorImpl<MCFixup> &Fixups) const = 0;
> +
> +  /// getAvailableFeatures - Return the current feature set, for use when an
> +  /// instruction is re-encoded later due to relaxation.
> +  virtual uint64_t getAvailableFeatures() const { return 0; }
> +
> +  /// setAvailableFeatures - Set the feature set to be used for subsequent
> +  /// EncodeInstruction calls.
> +  virtual void setAvailableFeatures(uint64_t Features) { }
> };
> 
> } // End llvm namespace
> diff --git a/lib/MC/MCAssembler.cpp b/lib/MC/MCAssembler.cpp
> index 68111f1..a2a73ea 100644
> --- a/lib/MC/MCAssembler.cpp
> +++ b/lib/MC/MCAssembler.cpp
> @@ -875,6 +875,7 @@ bool MCAssembler::relaxInstruction(MCAsmLayout &Layout,
>   SmallVector<MCFixup, 4> Fixups;
>   SmallString<256> Code;
>   raw_svector_ostream VecOS(Code);
> +  getEmitter().setAvailableFeatures(F.getEmitterFeatures());
>   getEmitter().EncodeInstruction(Relaxed, VecOS, Fixups);
>   VecOS.flush();
> 
> @@ -1093,6 +1094,7 @@ void MCFragment::dump() {
>     OS << "\n       ";
>     OS << " Inst:";
>     F->getInst().dump_pretty(OS);
> +    OS << " Features:" << F->getEmitterFeatures();
>     break;
>   }
>   case MCFragment::FT_Org:  {
> diff --git a/lib/MC/MCObjectStreamer.cpp b/lib/MC/MCObjectStreamer.cpp
> index 5c08610..5e03bf5 100644
> --- a/lib/MC/MCObjectStreamer.cpp
> +++ b/lib/MC/MCObjectStreamer.cpp
> @@ -242,6 +242,7 @@ void MCObjectStreamer::EmitInstToFragment(const MCInst &Inst) {
>   getAssembler().getEmitter().EncodeInstruction(Inst, VecOS, IF->getFixups());
>   VecOS.flush();
>   IF->getContents().append(Code.begin(), Code.end());
> +  IF->setEmitterFeatures(getAssembler().getEmitter().getAvailableFeatures());
> }
> 
> #ifndef NDEBUG
> diff --git a/lib/MC/MCPureStreamer.cpp b/lib/MC/MCPureStreamer.cpp
> index f7bf002..d60872f 100644
> --- a/lib/MC/MCPureStreamer.cpp
> +++ b/lib/MC/MCPureStreamer.cpp
> @@ -204,6 +204,7 @@ void MCPureStreamer::EmitInstToFragment(const MCInst &Inst) {
> 
>   IF->getContents() = Code;
>   IF->getFixups() = Fixups;
> +  IF->setEmitterFeatures(getAssembler().getEmitter().getAvailableFeatures());
> }
> 
> void MCPureStreamer::EmitInstToData(const MCInst &Inst) {
> diff --git a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
> index 25a3d4d..922e2e2 100644
> --- a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
> +++ b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp
> @@ -49,6 +49,13 @@ public:
> 
>   ~ARMMCCodeEmitter() {}
> 
> +  uint64_t getAvailableFeatures() const {
> +    return STI.getFeatureBits();
> +  }
> +  void setAvailableFeatures(uint64_t Features) {
> +    STI.ToggleFeature(STI.getFeatureBits() ^ Features);
> +  }
> +
>   bool isThumb() const {
>     // FIXME: Can tablegen auto-generate this?
>     return (STI.getFeatureBits() & ARM::ModeThumb) != 0;
> diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> index 576b6e0..2913c3f 100644
> --- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> +++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> @@ -42,6 +42,13 @@ public:
> 
>   ~X86MCCodeEmitter() {}
> 
> +  uint64_t getAvailableFeatures() const {
> +    return STI.getFeatureBits();
> +  }
> +  void setAvailableFeatures(uint64_t Features) {
> +    STI.ToggleFeature(STI.getFeatureBits() ^ Features);
> +  }
> +
>   bool is64BitMode() const {
>     // FIXME: Can tablegen auto-generate this?
>     return (STI.getFeatureBits() & X86::Mode64Bit) != 0;
> 
> 
> -- 
> dwmw2
>