[X86] Fix for bugzilla 31576 - add support for "data32" instruction prefix

[myatsina]

Summary

This patch fixes bugzilla 31576 (https://llvm.org/bugs/show_bug.cgi?id=31576).

"data32" instruction prefix was not defined in the llvm.
An exception had to be added to the X86 tablegen because both "data16" and "data32" are encoded to 0x66 (but in different modes).

[niravd]

Can you add some disassembler tests?

[myatsina]

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

[niravd]

In D28468#639720, @myatsina wrote:

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

[myatsina]

In D28468#641450, @niravd wrote:

In D28468#639720, @myatsina wrote:

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

Which kind of tests do you have in mind? Combination of "data16"/"data32" with additional instructions (like lgdt in the bug)? Some additional tests I'm missing?

[niravd]

In D28468#641481, @myatsina wrote:

In D28468#641450, @niravd wrote:

In D28468#639720, @myatsina wrote:

In D28468#639714, @niravd wrote:

Can you add some disassembler tests?

Do you mean to add tests for the tablegen diassembler change?

Yes. I also notice that the testsuite is short on testing data16/data32. It'd be nice to add a few more tests for these as well.

Which kind of tests do you have in mind? Combination of "data16"/"data32" with additional instructions (like lgdt in the bug)? Some additional tests I'm missing?

Tests to check the error messages for each an instruction (lgdt and it's explicitly sized variants should be fine) for the 16, 32, and 64 bit modes. gas will throw an error for "data16 ldgtw 0" but not for "data16 ldgtl 0". I don't think we need to throw an error for the former but we should have a test codifying the lack of failure for the second at least.

[myatsina]

Added relevant data16/data32+lgdt tests.

Regarding the disassembler tests - the llvm build itself is the test, if somebody breaks this disassembler tablegen change, the llvm build will fail.

[niravd]

Sorry I think I was being overly terse and unclear.

For the disassembler, I'd like to make sure we generate the correct output text for 16-bit mode emitted data prefixes.

That is, we should have an explicit test to check that:
echo ".byte 0x66" | as -32 -o /tmp/a.o
llvm-objdump -triple i386-unknown-unknown-code16 -d /tmp/a.o

should give us "data32" not "data16"

For the other tests, I was looking for tests that check for errors we use data16/32 in an invalid mode or in a redundant way. So for 32-bit "data32 lgdt 0" should emit an error (redundant) and data32 should not be valid in 64-bit mode.

[myatsina]

I've added the test that check for errors.

Regarding the disassembly, when data16/data32 is adjacent to an instruction like lgdt, then the encoding and the decoding is correct and the asm printer prints the right instruction. I've added disassembly tests for these cases.
When data16/data32 stands on its own the 0x66 byte by is disassembled correctly, but the asm printer always prints "data16" and not "data32" for the 16 bit mode.

I've found somebody already added an exception for CALLpcrel32 instruction in X86ATTInstPrinter because there's seems to be some lack of support of the "Requires" attribute.
I can add an exception there as well, but I'm not sure if this is the right thing to do because data16/data32 shouldn't really stand on it's own, it's a prefix to the instruction that follows.

[niravd]

Regarding the disassembly, when data16/data32 is adjacent to an instruction like lgdt, then the encoding and the decoding is correct and the asm printer prints the right instruction. I've added disassembly tests for these cases.
When data16/data32 stands on its own the 0x66 byte by is disassembled correctly, but the asm printer always prints "data16" and not "data32" for the 16 bit mode.

I've found somebody already added an exception for CALLpcrel32 instruction in X86ATTInstPrinter because there's seems to be some lack of support of the "Requires" attribute.
I can add an exception there as well, but I'm not sure if this is the right thing to do because data16/data32 shouldn't really stand on it's own, it's a prefix to the instruction that follows.

Oof. That's unfortunate. As ugly as it is, I think you should add the special case to like CALLpcrel32 just so we're certain we always generate code that doesn't trigger warnings/errors.

I still don't see the additional tests so make sure they're added, but otherwise LGTM.

[myatsina]

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

test/MC/X86/data-prefix-fail.s

8 ↗

(On Diff #84595)

This is the error checking test

test/MC/X86/data-prefix16.s

5 ↗

(On Diff #84595)

And this is one of the 3 disassembly tests (the other 2 are below).

[niravd]

In D28468#648513, @myatsina wrote:

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

Yes.

But I thought you also had one using llvm-objdump so we also test the object->instruction disassembly path.

[myatsina]

In D28468#648538, @niravd wrote:

In D28468#648513, @myatsina wrote:

Can you see the tests? I've added comments next to them.
Do this tests capture what you meant?

Yes.

But I thought you also had one using llvm-objdump so we also test the object->instruction disassembly path.

The llvm-objdump tests are the newly added data-prefix16.s, data-prefix32.s, data-prefix64.s which have the following run line:

RUN: llvm-mc -triple<triple> -filetype=obj %s -o - | llvm-objdump -triple <triple> -d - | FileCheck %s

I will add to those tests the "data16"/"data32" disassembly once I tweak the asm printer.

lib/Target/X86/AsmParser/X86AsmParser.cpp

bool X86AsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
 
   Operands.push_back(X86Operand::CreateToken(PatchedName, NameLoc));
 
   // Determine whether this is an instruction prefix.
   bool isPrefix =
     Name == "lock" || Name == "rep" ||
     Name == "repe" || Name == "repz" ||
     Name == "repne" || Name == "repnz" ||
-    Name == "rex64" || Name == "data16";
+    Name == "rex64" || Name == "data16" || Name == "data32";
 
   bool CurlyAsEndOfStatement = false;
   // This does the actual operand parsing.  Don't parse any more if we have a
   // prefix juxtaposed with an operation like "lock incl 4(%rax)", because we
   // just want to parse the "lock" as the first instruction and the "incl" as
   // the next one.
   if (getLexer().isNot(AsmToken::EndOfStatement) && !isPrefix) {
 

lib/Target/X86/X86InstrInfo.td

 // Lock instruction prefix
 def LOCK_PREFIX : I<0xF0, RawFrm, (outs),  (ins), "lock", []>;
 
 // Rex64 instruction prefix
 def REX64_PREFIX : I<0x48, RawFrm, (outs),  (ins), "rex64", []>,
                      Requires<[In64BitMode]>;
 
 // Data16 instruction prefix
-def DATA16_PREFIX : I<0x66, RawFrm, (outs),  (ins), "data16", []>;
+def DATA16_PREFIX : I<0x66, RawFrm, (outs),  (ins), "data16", []>, 
+                     Requires<[Not16BitMode]>;
+
+// Data instruction prefix
+def DATA32_PREFIX : I<0x66, RawFrm, (outs),  (ins), "data32", []>, 
+                     Requires<[In16BitMode]>;
 
 // Repeat string operation instruction prefixes
 // These uses the DF flag in the EFLAGS register to inc or dec ECX
 let Defs = [ECX], Uses = [ECX,EFLAGS] in {
 // Repeat (used with INS, OUTS, MOVS, LODS and STOS)
 def REP_PREFIX : I<0xF3, RawFrm, (outs),  (ins), "rep", []>;
 // Repeat while not equal (used with CMPS and SCAS)
 def REPNE_PREFIX : I<0xF2, RawFrm, (outs),  (ins), "repne", []>;

test/MC/X86/x86-16.s

 
 // CHECK: lretw
 // CHECK: encoding: [0xcb]
 lretw
 
 // CHECK: lretl
 // CHECK: encoding: [0x66,0xcb]
 lretl
+
+// CHECK: data32
+// CHECK: encoding: [0x66]
+data32
 No newline at end of file

utils/TableGen/X86DisassemblerTables.cpp

if (filter.accepts(index)) {
           InstructionSpecifiers[decision.instructionIDs[index]];
 
         if(previousInfo.name == "NOOP" && (newInfo.name == "XCHG16ar" ||
                                            newInfo.name == "XCHG32ar" ||
                                            newInfo.name == "XCHG32ar64" ||
                                            newInfo.name == "XCHG64ar"))
           continue; // special case for XCHG*ar and NOOP
 
+        if (previousInfo.name == "DATA16_PREFIX" &&
+            newInfo.name == "DATA32_PREFIX")
+          continue; // special case for data16 and data32
+
         if (outranks(previousInfo.insnContext, newInfo.insnContext))
           continue;
 
         if (previousInfo.insnContext == newInfo.insnContext) {
           errs() << "Error: Primary decode conflict: ";
           errs() << newInfo.name << " would overwrite " << previousInfo.name;
           errs() << "\n";
           errs() << "ModRM   " << index << "\n";