[PATCH v2 0/14] [x86] Fix 16-bit addressing modes (PR18220) and implement .code16 (PR8684)
David Woodhouse
dwmw2 at infradead.org
Fri Dec 20 17:06:40 PST 2013
On Fri, 2013-12-20 at 22:41 +0000, David Woodhouse wrote:
> On Fri, 2013-12-20 at 21:16 +0000, David Woodhouse wrote:
> >
> > I'm guessing the relocation fixups are happening after the .code16
> > switch has been processed? How does this work?
>
> Badly, it seems. Looks like the same problem would affect .code32
> vs. .code64 where instructions get re-encoded after a mode switch has
> been processed.
<snip C++-101 problem which I have now solved>
> Half-finished patch on top of my git tree (it only records the mode
> switches; doesn't actually do anything with the information):
And there lies a problem. I'm not sure what I *can* do with the
information. I need to "play back" the mode switches as we relax
instructions instructions, to make sure the code emitter is doing the
right thing. TI tried this, which looks vaguely sensible, and matches
what the EmitAssemblerFlag() method does:
@@ -968,6 +973,10 @@ bool MCAssembler::layoutSectionOnce(MCAsmLayout &Layout, MCSectionData &SD) {
case MCFragment::FT_LEB:
RelaxedFrag = relaxLEB(Layout, *cast<MCLEBFragment>(I));
break;
+ case MCFragment::FT_ModeSwitch:
+ getBackend().handleAssemblerFlag(cast<MCModeSwitchFragment>(I)->getMode());
+ break;
}
if (RelaxedFrag && !FirstRelaxedFragment)
FirstRelaxedFragment = I;
But telling the *backend* achieves nothing. The X86 backend doesn't even
*implement* handleAssemblerFlag(). The ARM one does, but even there the
code emitter doesn't have any way to *access* the information that the
backend keeps. The code emitter gets its idea of the mode from the
SubtargetInfo feature bits, which are directly manipulated by the
AsmParser and I can't see how to do that from here. If indeed we should.
And even if that wasn't the case, there's still a remaining issue with
this approach — we also have to reset the mode to the default (like
having an implicit .code32 or .code64 at the start of the file) when we
start processing the fragments.
I'm now looking at an alternative approach of actually encoding the
current mode *in* the MCInst so that when it's later relaxed, the
original mode is preserved. See below. This really is just a proof of
concept — I probably shouldn't just use the MCAssemblerFlag enum for
this purpose, and certainly shouldn't be abusing MCAF_SyntaxUnified in
this context to mean "not set", which will be the case for anything
*other* than parsed assembler code. But it does seem to work.
Suggestions welcome...
It was *so* naïve of me to think that fixing the 16-bit addressing modes
in .code32 was 90% of the way to fully-featured .code16 support :)
diff --git a/include/llvm/MC/MCInst.h b/include/llvm/MC/MCInst.h
index 4766815..f33563e 100644
--- a/include/llvm/MC/MCInst.h
+++ b/include/llvm/MC/MCInst.h
@@ -18,6 +18,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/MC/MCDirectives.h"
#include "llvm/Support/DataTypes.h"
#include "llvm/Support/SMLoc.h"
@@ -151,8 +152,9 @@ class MCInst {
unsigned Opcode;
SMLoc Loc;
SmallVector<MCOperand, 8> Operands;
+ MCAssemblerFlag Mode;
public:
- MCInst() : Opcode(0) {}
+ MCInst() : Opcode(0), Mode(MCAF_SyntaxUnified /* Anything but Code{16,32,64} */) {}
void setOpcode(unsigned Op) { Opcode = Op; }
unsigned getOpcode() const { return Opcode; }
@@ -160,6 +162,9 @@ public:
void setLoc(SMLoc loc) { Loc = loc; }
SMLoc getLoc() const { return Loc; }
+ void setMode(MCAssemblerFlag mode) { Mode = mode; }
+ MCAssemblerFlag getMode() const { return Mode; }
+
const MCOperand &getOperand(unsigned i) const { return Operands[i]; }
MCOperand &getOperand(unsigned i) { return Operands[i]; }
unsigned getNumOperands() const { return Operands.size(); }
diff --git a/lib/Target/X86/AsmParser/X86AsmParser.cpp b/lib/Target/X86/AsmParser/X86AsmParser.cpp
index 01746fa..4d6476f 100644
--- a/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ b/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -548,6 +548,14 @@ private:
// FIXME: Can tablegen auto-generate this?
return (STI.getFeatureBits() & X86::Mode16Bit) != 0;
}
+ MCAssemblerFlag currentMode() const {
+ if (is16BitMode())
+ return MCAF_Code16;
+ else if (is64BitMode())
+ return MCAF_Code64;
+ else
+ return MCAF_Code32;
+ }
void SwitchMode64() {
unsigned FB = ComputeAvailableFeatures(STI.ToggleFeature(X86::Mode64Bit));
setAvailableFeatures(FB);
@@ -2420,6 +2428,7 @@ MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
Op->getToken() == "finit" || Op->getToken() == "fsave" ||
Op->getToken() == "fstenv" || Op->getToken() == "fclex") {
MCInst Inst;
+ Inst.setMode(currentMode());
Inst.setOpcode(X86::WAIT);
Inst.setLoc(IDLoc);
if (!MatchingInlineAsm)
@@ -2443,6 +2452,7 @@ MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
bool WasOriginallyInvalidOperand = false;
MCInst Inst;
+ Inst.setMode(currentMode());
// First, try a direct match.
switch (MatchInstructionImpl(Operands, Inst,
diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
index 4392de2..313727a 100644
--- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -42,20 +42,26 @@ public:
~X86MCCodeEmitter() {}
- bool is64BitMode() const {
- // FIXME: Can tablegen auto-generate this?
- return (STI.getFeatureBits() & X86::Mode64Bit) != 0;
+ bool is64BitMode(const MCInst &MI) const {
+ if (MI.getMode() != MCAF_SyntaxUnified)
+ return MI.getMode() == MCAF_Code64;
+ else
+ return (STI.getFeatureBits() & X86::Mode64Bit) != 0;
}
- bool is32BitMode() const {
- // FIXME: Can tablegen auto-generate this?
- return (STI.getFeatureBits() & X86::Mode64Bit) == 0 &&
- (STI.getFeatureBits() & X86::Mode16Bit) == 0;
+ bool is32BitMode(const MCInst &MI) const {
+ if (MI.getMode() != MCAF_SyntaxUnified)
+ return MI.getMode() == MCAF_Code32;
+ else
+ return (STI.getFeatureBits() & X86::Mode64Bit) == 0 &&
+ (STI.getFeatureBits() & X86::Mode16Bit) == 0;
}
- bool is16BitMode() const {
- // FIXME: Can tablegen auto-generate this?
- return (STI.getFeatureBits() & X86::Mode16Bit) != 0;
+ bool is16BitMode(const MCInst &MI) const {
+ if (MI.getMode() != MCAF_SyntaxUnified)
+ return MI.getMode() == MCAF_Code16;
+ else
+ return (STI.getFeatureBits() & X86::Mode16Bit) != 0;
}
/// Is16BitMemOperand - Return true if the specified instruction has
@@ -65,7 +71,7 @@ public:
const MCOperand &IndexReg = MI.getOperand(Op+X86::AddrIndexReg);
const MCOperand &Disp = MI.getOperand(Op+X86::AddrDisp);
- if (is16BitMode() && BaseReg.getReg() == 0 &&
+ if (is16BitMode(MI) && BaseReg.getReg() == 0 &&
Disp.isImm() && Disp.getImm() < 0x10000)
return true;
if ((BaseReg.getReg() != 0 &&
@@ -387,7 +393,7 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst &MI, unsigned Op,
// Handle %rip relative addressing.
if (BaseReg == X86::RIP) { // [disp32+RIP] in X86-64 mode
- assert(is64BitMode() && "Rip-relative addressing requires 64-bit mode");
+ assert(is64BitMode(MI) && "Rip-relative addressing requires 64-bit mode");
assert(IndexReg.getReg() == 0 && "Invalid rip-relative address");
EmitByte(ModRMByte(0, RegOpcodeField, 5), CurByte, OS);
@@ -486,7 +492,7 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst &MI, unsigned Op,
BaseRegNo != N86::ESP &&
// If there is no base register and we're in 64-bit mode, we need a SIB
// byte to emit an addr that is just 'disp32' (the non-RIP relative form).
- (!is64BitMode() || BaseReg != 0)) {
+ (!is64BitMode(MI) || BaseReg != 0)) {
if (BaseReg == 0) { // [disp32] in X86-32 mode
EmitByte(ModRMByte(0, RegOpcodeField, 5), CurByte, OS);
@@ -1165,18 +1171,18 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte,
// The AdSize prefix is only for 32-bit and 64-bit modes; in 16-bit mode we
// need the address override only for JECXZ instead. Since it's only one
// instruction, we special-case it rather than introducing an AdSize16 bit.
- if ((!is16BitMode() && TSFlags & X86II::AdSize) ||
- (is16BitMode() && MI.getOpcode() == X86::JECXZ_32)) {
+ if ((!is16BitMode(MI) && TSFlags & X86II::AdSize) ||
+ (is16BitMode(MI) && MI.getOpcode() == X86::JECXZ_32)) {
need_address_override = true;
} else if (MemOperand == -1) {
need_address_override = false;
- } else if (is64BitMode()) {
+ } else if (is64BitMode(MI)) {
assert(!Is16BitMemOperand(MI, MemOperand));
need_address_override = Is32BitMemOperand(MI, MemOperand);
- } else if (is16BitMode()) {
+ } else if (is16BitMode(MI)) {
assert(!Is64BitMemOperand(MI, MemOperand));
need_address_override = !Is16BitMemOperand(MI, MemOperand);
- } else if (is32BitMode()) {
+ } else if (is32BitMode(MI)) {
assert(!Is64BitMemOperand(MI, MemOperand));
need_address_override = Is16BitMemOperand(MI, MemOperand);
} else {
@@ -1187,7 +1193,7 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte,
EmitByte(0x67, CurByte, OS);
// Emit the operand size opcode prefix as needed.
- if (TSFlags & (is16BitMode() ? X86II::OpSize16 : X86II::OpSize))
+ if (TSFlags & (is16BitMode(MI) ? X86II::OpSize16 : X86II::OpSize))
EmitByte(0x66, CurByte, OS);
bool Need0FPrefix = false;
@@ -1234,7 +1240,7 @@ void X86MCCodeEmitter::EmitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte,
// Handle REX prefix.
// FIXME: Can this come before F2 etc to simplify emission?
- if (is64BitMode()) {
+ if (is64BitMode(MI)) {
if (unsigned REX = DetermineREXPrefix(MI, TSFlags, Desc))
EmitByte(0x40 | REX, CurByte, OS);
}
--
dwmw2
More information about the llvm-commits
mailing list