[PATCHES] PR18303: Use appropriate Feature flags for encoding instructions
David Woodhouse
dwmw2 at infradead.org
Wed Jan 8 13:11:22 PST 2014
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.
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.
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
-------------- 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/20140108/12cdb91f/attachment.bin>
More information about the llvm-commits
mailing list