<div dir="ltr">LGTM. Do you have a commit access? Or would you like me to commit this?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 2, 2014 at 7:38 AM, David Woodhouse <span dir="ltr"><<a href="mailto:dwmw2@infradead.org" target="_blank">dwmw2@infradead.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, 2014-01-01 at 20:23 -0600, Craig Topper wrote:<br>
</div><div class="im">> Couple comments below. Otherwise LGTM.<br>
<br>
</div>I've addressed those and pushed the tree out to<br>
<a href="http://git.infradead.org/users/dwmw2/llvm.git" target="_blank">http://git.infradead.org/users/dwmw2/llvm.git</a> again; thanks.<br>
<br>
New version of the patch in question looks like this:<br>
<br>
>From 491cb8705fcfc4835ef2e67013e26256a050928e Mon Sep 17 00:00:00 2001<br>
From: David Woodhouse <<a href="mailto:David.Woodhouse@intel.com">David.Woodhouse@intel.com</a>><br>
Date: Thu, 12 Dec 2013 14:53:37 +0000<br>
Subject: [PATCH 01/12] Fix ModR/M byte output for 16-bit addressing modes<br>
(PR18220)<br>
<div class="im"><br>
Add some tests to validate correct register selection, including a fix<br>
to an existing test which was requiring the *wrong* output.<br>
---<br>
lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp | 60 ++++++++++++++++++++++++<br>
test/MC/X86/address-size.s | 8 +++-<br>
2 files changed, 67 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
</div>index 54a90f1..293541a 100644<br>
<div class="im">--- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
+++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
@@ -402,6 +402,66 @@ void X86MCCodeEmitter::EmitMemModRMByte(const MCInst &MI, unsigned Op,<br>
<br>
unsigned BaseRegNo = BaseReg ? GetX86RegNum(Base) : -1U;<br>
<br>
+ // 16-bit addressing forms of the ModR/M byte have a different encoding for<br>
+ // the R/M field and are far more limited in which registers can be used.<br>
+ if (Is16BitMemOperand(MI, Op)) {<br>
+ if (BaseReg) {<br>
+ // For 32-bit addressing, the row and column values in Table 2-2 are<br>
+ // basically the same. It's AX/CX/DX/BX/SP/BP/SI/DI in that order, with<br>
+ // some special cases. And GetX86RegNum reflects that numbering.<br>
+ // For 16-bit addressing it's more fun, as shown in the SDM Vol 2A,<br>
+ // Table 2-1 "16-Bit Addressing Forms with the ModR/M byte". We can only<br>
+ // use SI/DI/BP/BX, which have "row" values 4-7 in no particular order,<br>
+ // while values 0-3 indicate the allowed combinations (base+index) of<br>
+ // those: 0 for BX+SI, 1 for BX+DI, 2 for BP+SI, 3 for BP+DI.<br>
+ //<br>
+ // R16Table[] is a lookup from the normal RegNo, to the row values from<br>
+ // Table 2-1 for 16-bit addressing modes. Where zero means disallowed.<br>
</div>+ static const unsigned R16Table[] = { 0, 0, 0, 7, 0, 6, 4, 5 };<br>
<div class="im">+ unsigned RMfield = R16Table[BaseRegNo];<br>
+<br>
+ assert(RMfield && "invalid 16-bit base register");<br>
+<br>
+ if (IndexReg.getReg()) {<br>
+ unsigned IndexReg16 = R16Table[GetX86RegNum(IndexReg)];<br>
+<br>
+ assert(IndexReg16 && "invalid 16-bit index register");<br>
+ // We must have one of SI/DI (4,5), and one of BP/BX (6,7).<br>
+ assert(((IndexReg16 ^ RMfield) & 2) &&<br>
+ "invalid 16-bit base/index register combination");<br>
+ assert(Scale.getImm() == 1 &&<br>
+ "invalid scale for 16-bit memory reference");<br>
+<br>
+ // Allow base/index to appear in either order (although GAS doesn't).<br>
+ if (IndexReg16 & 2)<br>
+ RMfield = (RMfield & 1) | ((7 - IndexReg16) << 1);<br>
+ else<br>
+ RMfield = (IndexReg16 & 1) | ((7 - RMfield) << 1);<br>
+ }<br>
+<br>
+ if (Disp.isImm() && isDisp8(Disp.getImm())) {<br>
</div>+ if (Disp.getImm() == 0 && BaseRegNo != N86::EBP) {<br>
<div class="im">+ // There is no displacement; just the register.<br>
+ EmitByte(ModRMByte(0, RegOpcodeField, RMfield), CurByte, OS);<br>
+ return;<br>
+ }<br>
</div>+ // Use the [REG]+disp8 form, including for [BP] which cannot be encoded.<br>
<div class="im">+ EmitByte(ModRMByte(1, RegOpcodeField, RMfield), CurByte, OS);<br>
+ EmitImmediate(Disp, MI.getLoc(), 1, FK_Data_1, CurByte, OS, Fixups);<br>
+ return;<br>
</div>+ }<br>
+ // This is the [REG]+disp16 case.<br>
<div class="HOEnZb"><div class="h5">+ EmitByte(ModRMByte(2, RegOpcodeField, RMfield), CurByte, OS);<br>
+ } else {<br>
+ // There is no BaseReg; this is the plain [disp16] case.<br>
+ EmitByte(ModRMByte(0, RegOpcodeField, 6), CurByte, OS);<br>
+ }<br>
+<br>
+ // Emit 16-bit displacement for plain disp16 or [REG]+disp16 cases.<br>
+ EmitImmediate(Disp, MI.getLoc(), 2, FK_Data_2, CurByte, OS, Fixups);<br>
+ return;<br>
+ }<br>
+<br>
// Determine whether a SIB byte is needed.<br>
// If no BaseReg, issue a RIP relative instruction only if the MCE can<br>
// resolve addresses on-the-fly, otherwise use SIB (Intel Manual 2A, table<br>
diff --git a/test/MC/X86/address-size.s b/test/MC/X86/address-size.s<br>
index b105b40..936cd57 100644<br>
--- a/test/MC/X86/address-size.s<br>
+++ b/test/MC/X86/address-size.s<br>
@@ -8,6 +8,12 @@<br>
<br>
.code32<br>
movb $0x0, (%si)<br>
-// CHECK: encoding: [0x67,0xc6,0x06,0x00]<br>
+// CHECK: encoding: [0x67,0xc6,0x04,0x00]<br>
movb $0x0, (%esi)<br>
// CHECK: encoding: [0xc6,0x06,0x00]<br>
+ movw $0x1234, (%si)<br>
+// CHECK: encoding: [0x67,0x66,0xc7,0x04,0x34,0x12]<br>
+ movl $0x12345678, (%bx,%si,1)<br>
+// CHECK: encoding: [0x67,0xc7,0x00,0x78,0x56,0x34,0x12]<br>
+ movw $0x1234, 0x5678(%bp)<br>
+// CHECK: encoding: [0x67,0x66,0xc7,0x86,0x78,0x56,0x34,0x12]<br>
--<br>
1.8.3.1<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
dwmw2<br>
<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br>~Craig
</div>