thr3ads.net - llvm dev - [LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode [Dec 2013]

If this information is useful, please help other people find it:
Share via:

David Woodhouse

2013-Dec-12 16:36 UTC

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

This attempts to address http://llvm.org/bugs/show_bug.cgi?id=18220
It also fixes a test which was requiring the *wrong* output.

I'm relatively happy with this part, and it even solves most of the hard
part of feature request for .code16 in bug 8684 — which was actually why
I started prodding at this. But I could do with some help with the
16-bit signed relocation handling, which I've split into a subsequent
patch.

diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
index 7952607..12a30cf 100644
--- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -402,6 +402,56 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst
&MI, unsigned Op,
 
   unsigned BaseRegNo = BaseReg ? GetX86RegNum(Base) : -1U;
 
+  // 16-bit addressing forms of the ModR/M byte have a different encoding for
+  // the R/M field and are far more limited in which registers can be used.
+  if (Is16BitMemOperand(MI, Op)) {
+    if (BaseReg) {
+      // See Table 2-1 "16-Bit Addressing Forms with the ModR/M byte"
+      static const int R16Table[] = { 0, 0, 0, 7, 0, 6, 4, 5 };
+      unsigned RMfield = R16Table[BaseRegNo];
+
+      assert(RMfield && "invalid 16-bit base register");
+
+      if (IndexReg.getReg()) {
+        unsigned IndexReg16 = R16Table[GetX86RegNum(IndexReg)];
+
+        // Must have one of SI,DI (4,5), and one of BP/BX (6,7)
+        assert(((IndexReg16 ^ RMfield) & 2) &&
+               "invalid 16-bit base/index register combination");
+        assert(Scale.getImm() == 1 &&
+               "invalid scale for 16-bit memory reference");
+
+        if (IndexReg16 & 2)
+          RMfield = (RMfield & 1) | ((7 - IndexReg16) << 1);
+        else
+          RMfield = (IndexReg16 & 1) | ((7 - RMfield) << 1);
+      }
+
+      if (Disp.isImm() && isDisp8(Disp.getImm())) {
+        // Use [REG]+disp8 form if we can, and for [BP] which cannot be
encoded.
+        if (BaseRegNo == N86::EBP || Disp.getImm() != 0) {
+          EmitByte(ModRMByte(1, RegOpcodeField, RMfield), CurByte, OS);
+          EmitImmediate(Disp, MI.getLoc(), 1, FK_Data_1, CurByte, OS, Fixups);
+          return;
+        } else {
+          // No displacement
+          EmitByte(ModRMByte(0, RegOpcodeField, RMfield), CurByte, OS);
+          return;
+        }
+      }
+      EmitByte(ModRMByte(2, RegOpcodeField, RMfield), CurByte, OS);
+    } else {
+      // !BaseReg
+      EmitByte(ModRMByte(0, RegOpcodeField, 6), CurByte, OS);
+    }
+
+    // FIXME: Yes we can!
+    assert(Disp.isImm() && "cannot emit 16-bit relocation");
+    // Emit 16-bit displacement for plain disp16 or [REG]+disp16 cases.
+    EmitImmediate(Disp, MI.getLoc(), 2, FK_Data_2, CurByte, OS, Fixups);
+    return;
+  }
+
   // Determine whether a SIB byte is needed.
   // If no BaseReg, issue a RIP relative instruction only if the MCE can
   // resolve addresses on-the-fly, otherwise use SIB (Intel Manual 2A, table
diff --git a/test/MC/X86/address-size.s b/test/MC/X86/address-size.s
index b105b40..7b0bf6b 100644
--- a/test/MC/X86/address-size.s
+++ b/test/MC/X86/address-size.s
@@ -8,6 +8,6 @@
 
 	.code32
 	movb	$0x0, (%si)
-// CHECK: encoding: [0x67,0xc6,0x06,0x00]
+// CHECK: encoding: [0x67,0xc6,0x04,0x00]
 	movb	$0x0, (%esi)
 // CHECK: encoding: [0xc6,0x06,0x00]
-- 
1.8.3.1


-- 
                   Sent with MeeGo's ActiveSync support.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation



-------------- 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-dev/attachments/20131212/e78fa462/attachment.bin>

David Woodhouse

2013-Dec-12 16:54 UTC

head link

[LLVMdev] [RFC PATCH 2/2] x86: First attempt at supporting reloc_signed_2byte

Although this fixes all the test cases I've tested with so far, this is
almost certainly broken in a number of cases where the *signedness*
matters. Could really do with some help from someone with a bit more
clue about the details...

diff --git a/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
b/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
index ab95eb6..24c2bb1 100644
--- a/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
@@ -42,6 +42,7 @@ static unsigned getFixupKindLog2Size(unsigned Kind) {
   case FK_SecRel_1:
   case FK_Data_1: return 0;
   case FK_PCRel_2:
+  case X86::reloc_signed_2byte:
   case FK_SecRel_2:
   case FK_Data_2: return 1;
   case FK_PCRel_4:
@@ -88,6 +89,7 @@ public:
       { "reloc_riprel_4byte", 0, 4 * 8, MCFixupKindInfo::FKF_IsPCRel
},
       { "reloc_riprel_4byte_movq_load", 0, 4 * 8,
MCFixupKindInfo::FKF_IsPCRel},
       { "reloc_signed_4byte", 0, 4 * 8, 0},
+      { "reloc_signed_2byte", 0, 2 * 8, 0},
       { "reloc_global_offset_table", 0, 4 * 8, 0}
     };
 
diff --git a/lib/Target/X86/MCTargetDesc/X86ELFObjectWriter.cpp
b/lib/Target/X86/MCTargetDesc/X86ELFObjectWriter.cpp
index 3ddd865..c1be5ba 100644
--- a/lib/Target/X86/MCTargetDesc/X86ELFObjectWriter.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86ELFObjectWriter.cpp
@@ -89,6 +89,7 @@ unsigned X86ELFObjectWriter::GetRelocType(const MCValue
&Target,
           break;
         }
         break;
+      case X86::reloc_signed_2byte:
       case FK_PCRel_2:
         assert(Modifier == MCSymbolRefExpr::VK_None);
         Type = ELF::R_X86_64_PC16;
@@ -146,6 +147,15 @@ unsigned X86ELFObjectWriter::GetRelocType(const MCValue
&Target,
       case FK_Data_4:
         Type = ELF::R_X86_64_32;
         break;
+      case X86::reloc_signed_2byte:
+        switch (Modifier) {
+        default:
+          llvm_unreachable("Unimplemented");
+        case MCSymbolRefExpr::VK_None:
+          Type = ELF::R_X86_64_16;
+          break;
+        }
+        break;
       case FK_Data_2: Type = ELF::R_X86_64_16; break;
       case FK_PCRel_1:
       case FK_Data_1: Type = ELF::R_X86_64_8; break;
@@ -226,6 +236,7 @@ unsigned X86ELFObjectWriter::GetRelocType(const MCValue
&Target,
           break;
         }
         break;
+      case X86::reloc_signed_2byte:
       case FK_Data_2: Type = ELF::R_386_16; break;
       case FK_PCRel_1:
       case FK_Data_1: Type = ELF::R_386_8; break;
diff --git a/lib/Target/X86/MCTargetDesc/X86FixupKinds.h
b/lib/Target/X86/MCTargetDesc/X86FixupKinds.h
index f2e34cb..6ea3d70 100644
--- a/lib/Target/X86/MCTargetDesc/X86FixupKinds.h
+++ b/lib/Target/X86/MCTargetDesc/X86FixupKinds.h
@@ -20,6 +20,9 @@ enum Fixups {
   reloc_signed_4byte,                        // 32-bit signed. Unlike FK_Data_4
                                              // this will be sign extended at
                                              // runtime.
+  reloc_signed_2byte,                        // 16-bit signed. Unlike FK_Data_2
+                                             // this will be sign extended at
+                                             // runtime.
   reloc_global_offset_table,                 // 32-bit, relative to the start
                                              // of the instruction. Used only
                                              // for _GLOBAL_OFFSET_TABLE_.
diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
index 12a30cf..3f7ed26 100644
--- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -321,7 +321,8 @@ EmitImmediate(const MCOperand &DispOp, SMLoc Loc,
unsigned Size,
   // If we have an immoffset, add it to the expression.
   if ((FixupKind == FK_Data_4 ||
        FixupKind == FK_Data_8 ||
-       FixupKind == MCFixupKind(X86::reloc_signed_4byte))) {
+       FixupKind == MCFixupKind(X86::reloc_signed_4byte) ||
+       FixupKind == MCFixupKind(X86::reloc_signed_2byte))) {
     GlobalOffsetTableExprKind Kind = StartsWithGlobalOffsetTable(Expr);
     if (Kind != GOT_None) {
       assert(ImmOffset == 0);
@@ -331,13 +332,13 @@ EmitImmediate(const MCOperand &DispOp, SMLoc Loc,
unsigned Size,
         ImmOffset = CurByte;
     } else if (Expr->getKind() == MCExpr::SymbolRef) {
       if (HasSecRelSymbolRef(Expr)) {
-        FixupKind = MCFixupKind(FK_SecRel_4);
+        FixupKind = MCFixupKind(Size == 2 ? FK_SecRel_2 : FK_SecRel_4);
       }
     } else if (Expr->getKind() == MCExpr::Binary) {
       const MCBinaryExpr *Bin = static_cast<const MCBinaryExpr*>(Expr);
       if (HasSecRelSymbolRef(Bin->getLHS())
           || HasSecRelSymbolRef(Bin->getRHS())) {
-        FixupKind = MCFixupKind(FK_SecRel_4);
+        FixupKind = MCFixupKind(Size == 2 ? FK_SecRel_2 : FK_SecRel_4);
       }
     }
   }
@@ -445,10 +446,9 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst
&MI, unsigned Op,
       EmitByte(ModRMByte(0, RegOpcodeField, 6), CurByte, OS);
     }
 
-    // FIXME: Yes we can!
-    assert(Disp.isImm() && "cannot emit 16-bit relocation");
     // Emit 16-bit displacement for plain disp16 or [REG]+disp16 cases.
-    EmitImmediate(Disp, MI.getLoc(), 2, FK_Data_2, CurByte, OS, Fixups);
+    EmitImmediate(Disp, MI.getLoc(), 2, MCFixupKind(X86::reloc_signed_2byte),
CurByte, OS,
+                  Fixups);
     return;
   }
 
diff --git a/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
b/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
index 0f16621..8a9ac87 100644
--- a/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
+++ b/lib/Target/X86/MCTargetDesc/X86MachObjectWriter.cpp
@@ -86,6 +86,7 @@ static unsigned getFixupKindLog2Size(unsigned Kind) {
   case FK_PCRel_1:
   case FK_Data_1: return 0;
   case FK_PCRel_2:
+  case X86::reloc_signed_2byte:
   case FK_Data_2: return 1;
   case FK_PCRel_4:
     // FIXME: Remove these!!!
@@ -336,6 +337,9 @@ void
X86MachObjectWriter::RecordX86_64Relocation(MachObjectWriter *Writer,
         if (Kind == X86::reloc_signed_4byte)
           report_fatal_error("32-bit absolute addressing is not supported
in "
                              "64-bit mode", false);
+        else if (Kind == X86::reloc_signed_2byte)
+          report_fatal_error("16-bit absolute addressing is not supported
in "
+                             "64-bit mode", false);
       }
     }
   }
-- 
1.8.3.1



-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- 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-dev/attachments/20131212/8c42a15f/attachment.bin>

Eric Christopher

2013-Dec-16 19:46 UTC

head link

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

Hi David,

I'm catching up on email at the moment so I don't know if you've
done this,
but patches should go to llvm-commits for review if you wouldn't mind.

Thanks!

-eric

On Thu Dec 12 2013 at 8:39:19 AM, David Woodhouse <dwmw2 at infradead.org>
wrote:
> This attempts to address http://llvm.org/bugs/show_bug.cgi?id=18220
> It also fixes a test which was requiring the *wrong* output.
>
> I'm relatively happy with this part, and it even solves most of the
hard
> part of feature request for .code16 in bug 8684 — which was actually why
> I started prodding at this. But I could do with some help with the
> 16-bit signed relocation handling, which I've split into a subsequent
> patch.
>
> diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> index 7952607..12a30cf 100644
> --- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> +++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
> @@ -402,6 +402,56 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst
> &MI, unsigned Op,
>
>    unsigned BaseRegNo = BaseReg ? GetX86RegNum(Base) : -1U;
>
> +  // 16-bit addressing forms of the ModR/M byte have a different encoding
> for
> +  // the R/M field and are far more limited in which registers can be
> used.
> +  if (Is16BitMemOperand(MI, Op)) {
> +    if (BaseReg) {
> +      // See Table 2-1 "16-Bit Addressing Forms with the ModR/M
byte"
> +      static const int R16Table[] = { 0, 0, 0, 7, 0, 6, 4, 5 };
> +      unsigned RMfield = R16Table[BaseRegNo];
> +
> +      assert(RMfield && "invalid 16-bit base register");
> +
> +      if (IndexReg.getReg()) {
> +        unsigned IndexReg16 = R16Table[GetX86RegNum(IndexReg)];
> +
> +        // Must have one of SI,DI (4,5), and one of BP/BX (6,7)
> +        assert(((IndexReg16 ^ RMfield) & 2) &&
> +               "invalid 16-bit base/index register
combination");
> +        assert(Scale.getImm() == 1 &&
> +               "invalid scale for 16-bit memory reference");
> +
> +        if (IndexReg16 & 2)
> +          RMfield = (RMfield & 1) | ((7 - IndexReg16) << 1);
> +        else
> +          RMfield = (IndexReg16 & 1) | ((7 - RMfield) << 1);
> +      }
> +
> +      if (Disp.isImm() && isDisp8(Disp.getImm())) {
> +        // Use [REG]+disp8 form if we can, and for [BP] which cannot be
> encoded.
> +        if (BaseRegNo == N86::EBP || Disp.getImm() != 0) {
> +          EmitByte(ModRMByte(1, RegOpcodeField, RMfield), CurByte, OS);
> +          EmitImmediate(Disp, MI.getLoc(), 1, FK_Data_1, CurByte, OS,
> Fixups);
> +          return;
> +        } else {
> +          // No displacement
> +          EmitByte(ModRMByte(0, RegOpcodeField, RMfield), CurByte, OS);
> +          return;
> +        }
> +      }
> +      EmitByte(ModRMByte(2, RegOpcodeField, RMfield), CurByte, OS);
> +    } else {
> +      // !BaseReg
> +      EmitByte(ModRMByte(0, RegOpcodeField, 6), CurByte, OS);
> +    }
> +
> +    // FIXME: Yes we can!
> +    assert(Disp.isImm() && "cannot emit 16-bit
relocation");
> +    // Emit 16-bit displacement for plain disp16 or [REG]+disp16 cases.
> +    EmitImmediate(Disp, MI.getLoc(), 2, FK_Data_2, CurByte, OS, Fixups);
> +    return;
> +  }
> +
>    // Determine whether a SIB byte is needed.
>    // If no BaseReg, issue a RIP relative instruction only if the MCE can
>    // resolve addresses on-the-fly, otherwise use SIB (Intel Manual 2A,
> table
> diff --git a/test/MC/X86/address-size.s b/test/MC/X86/address-size.s
> index b105b40..7b0bf6b 100644
> --- a/test/MC/X86/address-size.s
> +++ b/test/MC/X86/address-size.s
> @@ -8,6 +8,6 @@
>
>         .code32
>         movb    $0x0, (%si)
> -// CHECK: encoding: [0x67,0xc6,0x06,0x00]
> +// CHECK: encoding: [0x67,0xc6,0x04,0x00]
>         movb    $0x0, (%esi)
>  // CHECK: encoding: [0xc6,0x06,0x00]
> --
> 1.8.3.1
>
>
> --
>                    Sent with MeeGo's ActiveSync support.
>
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse at intel.com                              Intel Corporation
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131216/bb66ceca/attachment.html>

David Woodhouse

2013-Dec-16 20:39 UTC

head link

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

On Mon, 2013-12-16 at 19:46 +0000, Eric Christopher
wrote:> 
> I'm catching up on email at the moment so I don't know if
you've done
> this, but patches should go to llvm-commits for review if you wouldn't
> mind.
I have done so. I've since worked out that the signed 16-bit relocation
is probably entirely pointless and I should just drop the assert() from
the first patch in the series, and drop the second patch entirely. And
I've gone a little further and have .code16 actually working for the
"test" case which provoked me into looking at it in the first place.
But
since nobody's responded, it didn't seem worth spamming the list with
another series just yet. I'll update my git tree at
http://git.infradead.org/users/dwmw2/llvm.git

-- 
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-dev/attachments/20131216/66d24a5a/attachment.bin>

Possibly Parallel Threads

Search for more seemingly similar threads

llvm dev - Dec 2013 - [LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

[LLVMdev] [RFC PATCH 2/2] x86: First attempt at supporting reloc_signed_2byte

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

[LLVMdev] [RFC PATCH 1/2] x86: Fix ModR/M byte output in 16-bit addressing mode

Possibly Parallel Threads