This is an archive of the discontinued LLVM Phabricator instance.

[X86][I86,I186,I286,I386,I486,PPRO, MMX]: Adding full coverage of MC encoding for the I86, I186, I286, I386, I486, PPRO and MMX isa sets.<NFC>
ClosedPublic

Authored by craig.topper on Dec 5 2017, 11:23 PM.

Details

Summary

NFC.
Adding MC regressions tests to cover the I86, I186, I286, I386, I486, PPRO and MMX isa sets.
This patch is part of a larger task to cover MC encoding of all X86 ISA Sets.
Started in revision: https://reviews.llvm.org/D39952

Diff Detail

Repository
rL LLVM

Event Timeline

gadi.haber created this revision.Dec 5 2017, 11:23 PM
craig.topper edited edge metadata.Dec 5 2017, 11:36 PM

Is breaking these up by generations that old even meaningful? Its pretty difficult these days to know when different instructions were added that pre-dated cpuid feature flags. Should we just have 2 files for this the -32 and -64?

Why are instructions like addw, xorw, etc. missing?

Why are there substantially more tests for xorb than xorl? Similar for addb vs addl?

I can combine them to a single file.
The reason for the separation is to improve the ability to track the coverage.
Note that there is also the PPRO ISA set.

gadi.haber updated this revision to Diff 125937.Dec 7 2017, 5:21 AM
  1. Following Craig's comment extended all I*86 tests.
  2. Added the tests for PENTIUMMMX and PPRO.

Still not seeing addw or xorw.

test/MC/X86/I386-64.s
869

tzcnt was added on haswell as part of BMI instructions.

test/MC/X86/PPRO-64.s
116

rdsspd is definitley not a pentium pro instruction. That's a control flow enhancement technology instruction that was just added.

Oh nevermind addw/xorw are there. Phabricator was collapsing the files

bound, and bswap seem to be missing. bswap is 486. Not sure about bound, but its pretty old i think. cpuid is also missing which should be 486.

What about lgdt, sgdt, lidt, sidt, lldt, sldt, lmsw, smsw, lsl, ltr, verr, verw, invlpg, lds, lss, les, lcs, lgs, lfs, arpl. pretty sure those are all 486 or older.

gadi.haber added inline comments.Dec 10 2017, 12:34 AM
test/MC/X86/PPRO-64.s
116

true. This is indeed a CET instruction. I will check how come it got added here,

Yes, The bswap belongs to I486REAL but I will add it to the I486 tests.
bound belongs to I186. I will check why it is missing from it.

Many of the remaining instructions you mentioned belong to I286. I will add them into this review as well.

Updated diff based on comments by Simon and Craig:

  1. Added the I286 and all Pentium tests
  2. Removed a CET instruction from PPRO tests.

Note: Some instructions are still missing from I186 tests (to be added in next diff update).

gadi.haber retitled this revision from [X86][I86][I186][I386][I486]: Adding full coverage of MC encoding for the I86, I186, I386 and I486 isa sets.<NFC> to [X86][I86,I186,I286,I386,I486,Pentium]: Adding full coverage of MC encoding for the I86, I186, I286, I386, I486 and Pentium isa sets.<NFC>.Dec 10 2017, 5:26 AM
gadi.haber edited the summary of this revision. (Show Details)
asb added a subscriber: asb.Dec 12 2017, 1:03 AM

You seem to have unwanted whitespace at the end of your CHECK lines (in both this patch and the previously committed patches).

I'd like to suggest an enhancement to this patch, the previously commited patches in this effort, and the other awaiting review: increase coverage by testing the "round-trip" of each instruction - i.e. check that instructions disassemble correctly as well by checking llvm-objdump output. You can see an example of this in the RISC-V backend (test/MC/RISCV/rv32i-valid.s for instance).

Extract below:

# RUN: llvm-mc %s -triple=riscv32 -show-encoding \
# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s

# CHECK-INST: sb a0, 2047(a2)
# CHECK: encoding: [0xa3,0x0f,0xa6,0x7e]
sb a0, 2047(a2)
# CHECK-INST: sh t3, -2048(t5)
# CHECK: encoding: [0x23,0x10,0xcf,0x81]
sh t3, -2048(t5)
# CHECK-INST: sh t3, -2048(t5)
# CHECK: encoding: [0x23,0x10,0xcf,0x81]
sh t3, %lo(2048)(t5)
# CHECK-INST: sw ra, 999(zero)
# CHECK: encoding: [0xa3,0x23,0x10,0x3e]
sw ra, 999(zero)

Interesting.
I can get/check the encoded bytes this way but what about checking the assembly instruction representation? e.g. checking the assembly instruction : "imull $0, %r13d, %r13d"?

asb added a comment.Dec 12 2017, 1:54 AM

Interesting.
I can get/check the encoded bytes this way but what about checking the assembly instruction representation? e.g. checking the assembly instruction : "imull $0, %r13d, %r13d"?

In the example I pasted I actually only check the assembly instruction in the objdump output.

You'd rewrite your test to do this:

// RUN: llvm-mc -triple i386-unknown-unknown --show-encoding %s | FileCheck -check-prefixes=CHECK,CHECK-INST %s
// RUN: llvm-mc -triple i386-unknown-unknown -filetype=obj < %s | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s

// CHECK-INST: enter $0, $0 
// CHECK: encoding: [0xc8,0x00,0x00,0x00]       
enter $0, $0 

// CHECK-INST: insb %dx, %es:(%edi) 
// CHECK: encoding: [0x6c]       
insb %dx, %es:(%edi)

The CHECK: line is ignored by the objdump RUN line, and CHECK-INST is checked for both.

Interesting. I will try it out.

Unfortunately, there are many cases of incompatibility tween what llvm-objdump generates and the disassembler output - causing the above tests to fail.
I will continue to investigate this as it is a good idea to test the llvm-objdump as well.
However, it may take a while and I may need to open different review for this

For the llvm-objdump problem, do the issues you're seeing look like bugs?

test/MC/X86/PENTIUM-64.s
1777 ↗(On Diff #126294)

Please put the MMX instructions in an MMX-64.s MMX-32.s test.

gadi.haber added inline comments.Dec 14 2017, 6:11 AM
test/MC/X86/PENTIUM-64.s
1777 ↗(On Diff #126294)

ok. will place them in separate tests

gadi.haber retitled this revision from [X86][I86,I186,I286,I386,I486,Pentium]: Adding full coverage of MC encoding for the I86, I186, I286, I386, I486 and Pentium isa sets.<NFC> to [X86][I86,I186,I286,I386,I486,PPRO, MMX]: Adding full coverage of MC encoding for the I86, I186, I286, I386, I486, PPRO and MMX isa sets.<NFC>.
gadi.haber edited the summary of this revision. (Show Details)

Updated diff following comments by Craig and Simon.

craig.topper commandeered this revision.Jan 11 2018, 8:17 PM
craig.topper edited reviewers, added: gadi.haber; removed: craig.topper.

Commandeering so I can rebase this patch

Remove 64-bit mode only instructions from the 32-bit tests. Use movq mnemonic instead of movd for moves between 64-bit GPRs and MMX registers.

This revision is now accepted and ready to land.Jan 15 2018, 11:43 AM
This revision was automatically updated to reflect the committed changes.
test/MC/X86/I486-64.s