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
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
- Following Craig's comment extended all I*86 tests.
- Added the tests for PENTIUMMMX and PPRO.
Comment Actions
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.
| test/MC/X86/PPRO-64.s | ||
|---|---|---|
| 115 | true. This is indeed a CET instruction. I will check how come it got added here, | |
Comment Actions
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.
Comment Actions
Updated diff based on comments by Simon and Craig:
- Added the I286 and all Pentium tests
- Removed a CET instruction from PPRO tests.
Note: Some instructions are still missing from I186 tests (to be added in next diff update).
Comment Actions
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)
Comment Actions
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"?
Comment Actions
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.
Comment Actions
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
Comment Actions
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. |
| test/MC/X86/PENTIUM-64.s | ||
|---|---|---|
| 1777 ↗ | (On Diff #126294) | ok. will place them in separate tests |
Comment Actions
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.
tzcnt was added on haswell as part of BMI instructions.