Microsoft compatibility – add support for “relaxation” of memory operands in inline assembly.

[m_zuckerman]

Summary

Microsoft compiler performs “relaxation” of the X86 instructions and expands the set of legal instruction forms that can be used in the X86 inline assembly.
Microsoft achieves this by transforming instructions that have an illegal form according to X86 spec into a legal one.
For example, the illegal instruction form:
mov r16, m32
Is transformed into the following legal form (based on the type of the register operand):
mov r16, m16

There are many instructions that have such “relaxations” and they are all needed in order for llvm to be consistent with Microsoft compiler.

[t.p.northover]

I don't think this should be supported outside a specific Microsoft mode (and not there if we can possibly avoid it). Those instructions are simply wrong, and should be diagnosed. Many of them even have two equally likely valid interpretations.

Tim.

[myatsina]

I agree with you that there are several possible interpretations for many of the instructions, and we chose only one interpretation - the one that allows Microsoft compatibility.

We found a very large number of instructions (>500) for which Microsoft compiler performs such "relaxations", therefore, patching the X86AsmParser with all these special cases seems as an unreasonable solution for such a wide-spread problem. We chose to use the already exiting td infrastructure and refine the instructions already defined in it.

The changes in this review affect only the intel syntax assembly, so the relaxations are somewhat contained (in at&t syntax you do not specify the sizes of each pointer operand, you just add a size suffix to the instruction, therefore, you don’t encounter such situations where there is a mismatch between the operands).

Do you have a suggestion how the definitions in the td files can be further contained to "Microsoft only" syntax? Or perhaps, do you have another suggestion for an infrastructure that will allow adding support for so many “relaxations” without the need to repeat the instruction definitions that already exist in the td files?

[t.p.northover]

I agree with you that there are several possible interpretations for many of the instructions, and we chose only one interpretation - the one that allows Microsoft compatibility.

It goes beyond a simple ambiguity. This patch takes an explicitly self-contradictory instruction, discards an arbitrary part of what the programmer wrote and ploughs on regardless. That's simply a buggy assembler, not a "relaxation" in any conventional sense. I'd hope even most programmers targeting Microsoft platforms would prefer to be told that they'd made that kind of mistake. I certainly would on Linux or OSX.

As for achieving compatibility without breaking other OSs, I'm not sure. Maybe introduce a third syntax, beyond AT&T and Intel (I'd be as snarky as possible about the name, personally "ms_buggy" perhaps. "ms_compat" would be a more diplomatic choice). A modified ParseIntelMemOperand could then put a field in MemOp effectively saying "this was parsed from potentially buggy source", which isBuggyMem8 could then check for.

It's really ugly, and I wouldn't be surprised if there was a better way. But then I wouldn't be surprised if there wasn't, either.

Tim.

[rnk]

Is this something that masm does, or something that cl does?

Is the goal here to make LLVM's assembler accept everything masm accepts, or to help it accept inline assembly coming from Clang?

[myatsina]

We've been checking the inline assembly against cl.exe and cl.exe accepts these expansions and turns them into legal instructions.

The goal is Microsoft compatibility - make llvm accept what Microsoft accepts.

We're looking into Tim's proposal of introducing a third syntax.

[rnk]

In D12399#237111, @myatsina wrote:

We've been checking the inline assembly against cl.exe and cl.exe accepts these expansions and turns them into legal instructions.

The goal is Microsoft compatibility - make llvm accept what Microsoft accepts.

I want to know if masm behaves differently from MSVC here. I would like to make this logic conditional on parsing inline intel assembly if possible.

For a standalone assembly files, we have a bit more leeway to remain strict. There aren't as many cargo-culted assembly system headers as there are for C/C++.

We're looking into Tim's proposal of introducing a third syntax.

[m_zuckerman]

This comment was removed by m_zuckerman.

[m_zuckerman]

First, we all agree this is a point of view issue , Strict sence vs wide sence .

I'd hope even most programmers targeting Microsoft platforms would prefer to be told that they'd made that kind of mistake.

I agree with you that the compiler should alert the programmers when they use these “exapnsions”.

I want to know if masm behaves differently from MSVC here.

Yes masm behaves differently. In MSVC you can write illegal memory size (by spec) and get the correct asm. In llvm this is not supported , Only spec instrucations .

I would like to make this logic conditional on parsing inline intel assembly if possible.

I checked t.p.northover suggestion.

a. To apply third syntax we need to create third table matcher (att,intel,MS_compat):
To do so with the exist infrastructure. We need to change all the 20 X86instr td file and more than 10000 instructions to accepting the third syntax (Change {att|intel} to {att|instel|MS_comp} ).
We can do it , but this is the cost. The impact is bigger than before.

[rnk]

If this is really an MSVC inline assembly compatibility issue, then lets try to fix it in the inline assembly parser. Can you give examples of the inline assembly that users are actually writing? I imagine that the inline assembly parser may be incorrectly infering memory operand sizes here, like in this code:

int g;
void f() {
  __asm mov g, ax
}

I think somtimes clang/llvm says that 'r' is a dword ptr even though the user didn't write that. If users are actually writing mov ax, dword ptr g I'm pretty comfortable with diagnosing it.

[m_zuckerman]

In D12399#237755, @rnk wrote:

If this is really an MSVC inline assembly compatibility issue, then lets try to fix it in the inline assembly parser. Can you give examples of the inline assembly that users are actually writing? I imagine that the inline assembly parser may be incorrectly infering memory operand sizes here, like in this code:

int g;
void f() {
  __asm mov g, ax
}

I think somtimes clang/llvm says that 'r' is a dword ptr even though the user didn't write that. If users are actually writing mov ax, dword ptr g I'm pretty comfortable with diagnosing it.

I ran some tests to compare the MASM and cl.exe ( MSVC ) . As I wrote MASM behaves differently from MSVC . For example, I use BTC instruction. This is for MSVC
char test_mem[16];
int main(void)
{
__asm {

btc xmmword ptr test_mem, 8
 btc xmmword ptr test_mem, eax
 btc xmmword ptr test_mem, eax

}
return 0;
}
This is the asm output:
char test_mem[16];

int main(void)
{
push ebp
mov ebp,esp
sub esp,0C0h
push ebx
push esi
push edi
lea edi,[ebp-0C0h]
mov ecx,30h
mov eax,0CCCCCCCCh
rep stos dword ptr es:[edi]

__asm {
  btc xmmword ptr test_mem, 8

btc word ptr ds:[3B8130h],8

btc xmmword ptr test_mem, eax

btc dword ptr ds:[3B8130h],eax

btc xmmword ptr test_mem, eax

btc dword ptr ds:[3B8130h],eax

}
return 0;

xor eax,eax
}

When I tried to run it on MASM. The MASM returned with an error massage

C:\masm32\bin>ml C:\work\mzuckerm\RunProj\run.asm
Microsoft (R) Macro Assembler Version 6.14.8444
Copyright (C) Microsoft Corp 1981-1997. All rights reserved.

Assembling: C:\work\mzuckerm\RunProj\run.asm
C:\work\mzuckerm\RunProj\run.asm(17) : error A2006: undefined symbol : xmmword
C:\work\mzuckerm\RunProj\run.asm(18) : error A2006: undefined symbol : xmmword
C:\work\mzuckerm\RunProj\run.asm(19) : error A2006: undefined symbol : xmmword

As you can see the MASM behaves differently from MSVC

[myatsina]

I want to emphasize that there are hundreds of instructions that have these "relaxations", therefore, I think that fixing all of them in the X86AsmParser is not a desired option.

The question remains, what options do we have to support this? Adding a new "MS_compt" syntax to the td files has a high cost on all the td files and all the instructions.

[rnk]

In D12399#240721, @myatsina wrote:

I want to emphasize that there are hundreds of instructions that have these "relaxations", therefore, I think that fixing all of them in the X86AsmParser is not a desired option.

The question remains, what options do we have to support this? Adding a new "MS_compt" syntax to the td files has a high cost on all the td files and all the instructions.

It's not clear to me that we need to enumerate all of the instructions to solve this problem. This seems like it's really about ignoring memory operand sizes of source-level identifiers. That seems like a pretty non-invasive change to the assembly parser.

[m_zuckerman]

In D12399#237685, @m_zuckerman wrote:

a. To apply third syntax we need to create third table matcher (att,intel,MS_compat):
To do so with the exist infrastructure. We need to change all the 20 X86instr td file and more than 10000 instructions to accepting the third syntax (Change {att|intel} to {att|instel|MS_comp} ).
We can do it , but this is the cost. The impact is bigger than before.

We need support from the table matcher to do mapping from not support instruction to support instruction.

For example BTC:
This the spec of BTC- bit test and complement ( intel spec )

Opcode	Instruction
1. 0F BB /r	BTC r/m16,r16
2. 0F BB /r	BTC r/m32,r32
3. Rex.w+0F BB /r	BTC r/m64,r64
4. 0F BA /7 ib	BTC r/m16,imm8
5. 0F BA /7 ib	BTC r/m32,imm8
6. REX.W + 0F BA /7 ib	BTC r/m64,imm8

1. We want to continue to print the supported memory size with the correct opcode (as in spec). For memory size , word- qword still the print needed to be as spec.
2. For memory size that not supported in the spec (byte , tbyte, xmmword ) that not yet supported by inline asm. MSVC map tbye and xmmword to the lowest memory size spec ( word)
3. What happen if we use BTC xmmword ptr test_mem , imm8 which of the 4-6 instruction we will chose. MSVC again chose the minimum instruction (4).

MSVC ASM output
the bold is input
char test_mem[16];

void main (void)
{
push ebp
mov ebp,esp
sub esp,0C0h
push ebx
push esi
push edi
lea edi,[ebp+FFFFFF40h]
mov ecx,30h
mov eax,0CCCCCCCCh
rep stos dword ptr es:[edi]

__asm {

btc XMMWORD PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc TBYTE PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc BYTE PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc XMMWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc QWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc TBYTE PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc DWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc BYTE PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc XMMWORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc QWORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc TBYTE PTR test_mem,eax
btc dword ptr ds:[00E38130h],eax
btc WORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc BYTE PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax

}

}

From that I understand we need to create table. That looking for the minimum supported memory. I don’t ignore the memory size. Only not supported memory I ignore and mapping the "ignore" memory to the correct size. In each instruction the minimum size is something else. For that again we need table.

Another example is DEC instruction where the opcode is different from one memory to the other memory. And the minimum memory size here different than the btc.

Opcode	Instruction
1. FE /1	DEC r/m8
2. FF /1	DEC r/m16
3. FF /1	DEC r/m32
4. REX.W+ FF /1	DEC r/m64

bold is input
__asm {
dec QWORD PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec DWORD PTR test_mem
FF 0D 30 81 1D 00 dec dword ptr ds:[001D8130h]
dec WORD PTR test_mem
66 FF 0D 30 81 1D 00 dec word ptr ds:[001D8130h]
dec BYTE PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec XMMWORD PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec TBYTE PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]

}

}

In conclusion we need to store the information of the minimum memory size for each instruction. For that we will need a table.

[rnk]

In D12399#240955, @m_zuckerman wrote:

In conclusion we need to store the information of the minimum memory size for each instruction. For that we will need a table.

The table already has information about what memory operand sizes each instruction supports.

I'm suggesting that instead of modifying the data in the table, you modify the matcher. Take a look at how we deal with unsized memory operands in X86AsmParser::MatchAndEmitIntelInstruction(). It sounds that logic would solve your problem if we just dropped the user-provided memory operand size on the floor.

The next step is to narrow down the conditions under which we need to do this. I suspect that MSVC ignores user provided operand sizes when a source-level identifier is involved.

[majnemer]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2670–2672 ↗

(On Diff #38422)

Would it make more sense to predicate this on whether or not we are doing inline assembly instead of whether or not our OS is Windows?

[rnk]

There are more than a few spelling mistakes in this patch, so I'd appreciate it if you ran spellcheck first. Also consider reformatting the C++ code with clang-format. The instructions for using it on your patch are here: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

lib/Target/X86/AsmParser/X86AsmParser.cpp

2672 ↗

(On Diff #38422)

I think the right condition here is not "are we targeting Windows" but "are we parsing Intel inline assembly".

test/MC/X86/relaxation.s

1 ↗

(On Diff #38422)

Given my comment above about making this conditional on inline assembly, you will have to make this a clang test for now.

2–7 ↗

(On Diff #38422)

This comment isn't needed, this test isn't different from the other tests in this directory.

[t.p.northover]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2670–2672 ↗

(On Diff #38422)

I'm still keen to limit it to Windows somehow. I don't think people writing Intel inline assembly elsewhere will want wrong access size to be silently ignored: either of the contradictory specifiers could be the one intended.

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2670–2672 ↗

(On Diff #38422)

It seems like we really want this to be protected by -fms-compatibility. I wonder if we should thread that flag through to the assembler.

[m_zuckerman]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2670–2672 ↗

(On Diff #38422)

I agree with you about the -fms-compatability. Regarding your saying, “It seems like we really want this to be protected by -fms-compatibility. I wonder if we should thread that flag through to the assembler”, I’ve checked it, and lowering this flag to LLVM is currently not supported by clang. If you know of any way to do it correctly, please let me know. I believe we should first commit the general logic that supports these MS “relaxations” using the triple, and then, at the second stage we should discuss (with clang guys as well) how to connect the “fms-compatability” to this logic instead of the triple.

Unlike “fms-compatability”, the triple is available in the X86AsmParser, and nobody except for users that use intel syntax inlineasm under the Microsoft triple will be affected by this.

[rnk]

Honestly, I don't see the point of doing this. Is there some popular library that needs this compatibility hack? If not, I think we should just tell users to fix their code.

[m_zuckerman]

I am not familiar with any libraries that contain these “relaxations”. Never the less Microsoft supports these “relaxations”. If we want LLVM to be compatible with Microsoft like GAS, we need to add this feature. I don’t think that “just tell users to fix their code” is the correct approach, but we definitely should warn them.
I’ve added support for –fms-compatibility for the “relaxations” to work. The user needs to pass two flags: fams-blocks and fms-compatibility.

lib/Target/X86/X86InstrCMovSetCC.td

def NAME#64rr
           [(set GR64:$dst,
                 (X86cmov GR64:$src1, GR64:$src2, CondNode, EFLAGS))],
                 IIC_CMOV32_RR>, TB;
   }
 
   let Uses = [EFLAGS], Predicates = [HasCMov], Constraints = "$src1 = $dst",
       SchedRW = [WriteALULd, ReadAfterLd] in {
     def NAME#16rm
-      : I<opc, MRMSrcMem, (outs GR16:$dst), (ins GR16:$src1, i16mem:$src2),
+      : I<opc, MRMSrcMem, (outs GR16:$dst), (ins GR16:$src1, anyi16mem:$src2),
           !strconcat(Mnemonic, "{w}\t{$src2, $dst|$dst, $src2}"),
           [(set GR16:$dst, (X86cmov GR16:$src1, (loadi16 addr:$src2),
                                     CondNode, EFLAGS))], IIC_CMOV16_RM>,
                                     TB, OpSize16;
     def NAME#32rm
-      : I<opc, MRMSrcMem, (outs GR32:$dst), (ins GR32:$src1, i32mem:$src2),
+      : I<opc, MRMSrcMem, (outs GR32:$dst), (ins GR32:$src1, anyi32mem:$src2),
           !strconcat(Mnemonic, "{l}\t{$src2, $dst|$dst, $src2}"),
           [(set GR32:$dst, (X86cmov GR32:$src1, (loadi32 addr:$src2),
                                     CondNode, EFLAGS))], IIC_CMOV32_RM>,
                                     TB, OpSize32;
     def NAME#64rm
-      :RI<opc, MRMSrcMem, (outs GR64:$dst), (ins GR64:$src1, i64mem:$src2),
+      :RI<opc, MRMSrcMem, (outs GR64:$dst), (ins GR64:$src1, anyi64mem:$src2),
           !strconcat(Mnemonic, "{q}\t{$src2, $dst|$dst, $src2}"),
           [(set GR64:$dst, (X86cmov GR64:$src1, (loadi64 addr:$src2),
                                     CondNode, EFLAGS))], IIC_CMOV32_RM>, TB;
   } // Uses = [EFLAGS], Predicates = [HasCMov], Constraints = "$src1 = $dst"
 } // end multiclass
 
 
 // Conditional Moves.

 
 // SetCC instructions.
 multiclass SETCC<bits<8> opc, string Mnemonic, PatLeaf OpNode> {
   let Uses = [EFLAGS] in {
     def r    : I<opc, MRMXr,  (outs GR8:$dst), (ins),
                      !strconcat(Mnemonic, "\t$dst"),
                      [(set GR8:$dst, (X86setcc OpNode, EFLAGS))],
                      IIC_SET_R>, TB, Sched<[WriteALU]>;
-    def m    : I<opc, MRMXm,  (outs), (ins i8mem:$dst),
+    def m    : I<opc, MRMXm,  (outs), (ins anyi8mem:$dst),
                      !strconcat(Mnemonic, "\t$dst"),
                      [(store (X86setcc OpNode, EFLAGS), addr:$dst)],
                      IIC_SET_M>, TB, Sched<[WriteALU, WriteStore]>;
   } // Uses = [EFLAGS]
 }
 
 defm SETO  : SETCC<0x90, "seto",  X86_COND_O>;   // is overflow bit set
 defm SETNO : SETCC<0x91, "setno", X86_COND_NO>;  // is overflow bit not set

lib/Target/X86/X86InstrInfo.td

 // Gather mem operands
 class X86VMemOperand<RegisterClass RC, string printMethod,
                      AsmOperandClass parserMatchClass>
     : X86MemOperand<printMethod, parserMatchClass> {
   let MIOperandInfo = (ops ptr_rc, i8imm, RC, i32imm, i8imm);
 }
 
 def anymem : X86MemOperand<"printanymem">;
+def anyi8mem  : X86MemOperand<"printi8mem">;
+def anyi16mem : X86MemOperand<"printi16mem">;
+def anyi32mem : X86MemOperand<"printi32mem">;
+def anyi64mem : X86MemOperand<"printi64mem">;
 
 def opaque32mem : X86MemOperand<"printopaquemem">;
 def opaque48mem : X86MemOperand<"printopaquemem">;
 def opaque80mem : X86MemOperand<"printopaquemem">;
 def opaque512mem : X86MemOperand<"printopaquemem">;
 
 def i8mem   : X86MemOperand<"printi8mem",   X86Mem8AsmOperand>;
 def i16mem  : X86MemOperand<"printi16mem",  X86Mem16AsmOperand>;

test/MC/X86/msCompatibility.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel -output-asm-variant=1 %s | FileCheck %s 
+
+//CHECK:cmova ax, word ptr [test_mem] 
+cmova ax,XMMWORD PTR test_mem
+//CHECK:cmova ax, word ptr [test_mem] 
+cmova ax,QWORD PTR test_mem
+//CHECK:cmova ax, word ptr [test_mem] 
+cmova ax,TBYTE PTR test_mem
+//CHECK:cmova ax, word ptr [test_mem] 
+cmova ax,DWORD PTR test_mem
+//CHECK:cmova ax, word ptr [test_mem] 
+cmova ax,BYTE PTR test_mem
+//CHECK:cmova eax, dword ptr [test_mem] 
+cmova eax,XMMWORD PTR test_mem
+//CHECK:cmova eax, dword ptr [test_mem] 
+cmova eax,QWORD PTR test_mem
+//CHECK:cmova eax, dword ptr [test_mem] 
+cmova eax,TBYTE PTR test_mem
+//CHECK:cmova eax, dword ptr [test_mem] 
+cmova eax,WORD PTR test_mem
+//CHECK:cmova eax, dword ptr [test_mem] 
+cmova eax,BYTE PTR test_mem
+//CHECK:cmova rax, qword ptr [test_mem] 
+cmova rax,XMMWORD PTR test_mem
+//CHECK:cmova rax, qword ptr [test_mem] 
+cmova rax,TBYTE PTR test_mem
+//CHECK:cmova rax, qword ptr [test_mem] 
+cmova rax,DWORD PTR test_mem
+//CHECK:cmova rax, qword ptr [test_mem] 
+cmova rax,WORD PTR test_mem
+//CHECK:cmova rax, qword ptr [test_mem] 
+cmova rax,BYTE PTR test_mem

utils/TableGen/X86RecognizableInstr.cpp

OperandType RecognizableInstr::typeFromString(const std::string &s,
   TYPE("brtarget16",          TYPE_RELv)
   TYPE("brtarget8",           TYPE_REL8)
   TYPE("f80mem",              TYPE_M80FP)
   TYPE("lea64_32mem",         TYPE_LEA)
   TYPE("lea64mem",            TYPE_LEA)
   TYPE("VR64",                TYPE_MM64)
   TYPE("i64imm",              TYPE_IMMv)
   TYPE("anymem",              TYPE_M)
+  TYPE("anyi8mem",            TYPE_M)
+  TYPE("anyi16mem",           TYPE_M)
+  TYPE("anyi32mem",           TYPE_M)
+  TYPE("anyi64mem",           TYPE_M)
   TYPE("opaque32mem",         TYPE_M1616)
   TYPE("opaque48mem",         TYPE_M1632)
   TYPE("opaque80mem",         TYPE_M1664)
   TYPE("opaque512mem",        TYPE_M512)
   TYPE("SEGMENT_REG",         TYPE_SEGMENTREG)
   TYPE("DEBUG_REG",           TYPE_DEBUGREG)
   TYPE("CONTROL_REG",         TYPE_CONTROLREG)
   TYPE("srcidx8",             TYPE_SRCIDX8)
RecognizableInstr::memoryEncodingFromString(const std::string &s,
   ENCODING("f32mem",          ENCODING_RM)
   ENCODING("i128mem",         ENCODING_RM)
   ENCODING("i256mem",         ENCODING_RM)
   ENCODING("i512mem",         ENCODING_RM)
   ENCODING("f80mem",          ENCODING_RM)
   ENCODING("lea64_32mem",     ENCODING_RM)
   ENCODING("lea64mem",        ENCODING_RM)
   ENCODING("anymem",          ENCODING_RM)
+  ENCODING("anyi8mem",        ENCODING_RM)
+  ENCODING("anyi16mem",       ENCODING_RM)
+  ENCODING("anyi32mem",       ENCODING_RM)
+  ENCODING("anyi64mem",       ENCODING_RM)
   ENCODING("opaque32mem",     ENCODING_RM)
   ENCODING("opaque48mem",     ENCODING_RM)
   ENCODING("opaque80mem",     ENCODING_RM)
   ENCODING("opaque512mem",    ENCODING_RM)
   ENCODING("vx32mem",         ENCODING_RM)
   ENCODING("vx32xmem",        ENCODING_RM)
   ENCODING("vy32mem",         ENCODING_RM)
   ENCODING("vy32xmem",        ENCODING_RM)