Looking at D49861 Roman discovered that we don't have MCA tests for XCHG* instrs. This patch fixes this issue.
Details
Diff Detail
Event Timeline
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
762–763 | http://felixcloutier.com/x86/XCHG.html suggests that memory operand can be on either side, |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
762–763 | In fact I checked it but decided not to include in the test because this test is not about syntax. But if you insist I'll extend the test. Is it enough to include the check in one test file only? I mean that I'd like to check this syntax construction for one cpu, OK? Or you'd like to see it inside all 10 files? |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
762–763 |
I agree. But the existing tests are doing exhaustive coverage - e.g. see sub, add.
All these generic files should be identical other than the CPU specified in run line. So yes, all of them, please. |
Thank you for adding this test coverage.
It looks about right to me, but i don't have enough expirience with these MCA tests to accept this.
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
760 | I don't think only byte ops have special case AL instructions? It's just ax/eax/rax according to APMv3 | |
762–763 | Do they actually have different encodings? I just thought they were commutable in the assembly but would generate the same encoding - @craig.topper do you know? |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
230 | Don't use AL/AX/EAX/RAX as explicit ops - they are implicitly referenced by in the instruction | |
233 | cmpxchg8b/cmpxchg16b tests were added at rL338595 (in their own file) - please remove them from this patch | |
762–763 | According to X86InstrInfo.td: // xchg: We accept "xchgX <reg>, <mem>" and "xchgX <mem>, <reg>" as synonyms. def : InstAlias<"xchg{b}\t{$mem, $val|$val, $mem}", (XCHG8rm GR8 :$val, i8mem :$mem), 0>; def : InstAlias<"xchg{w}\t{$mem, $val|$val, $mem}", (XCHG16rm GR16:$val, i16mem:$mem), 0>; def : InstAlias<"xchg{l}\t{$mem, $val|$val, $mem}", (XCHG32rm GR32:$val, i32mem:$mem), 0>; def : InstAlias<"xchg{q}\t{$mem, $val|$val, $mem}", (XCHG64rm GR64:$val, i64mem:$mem), 0>; So we shouldn't need the commuted variants. |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
762–763 | Yeah its just a parser trick. The encoding is the same. |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
230 | It seems it deos not work: <stdin>:225:1: error: too few operands for instruction ^~~~~~ <stdin>:228:1: error: too few operands for instruction ^~~~~~ <stdin>:231:1: error: too few operands for instruction ^~~~~~ <stdin>:234:1: error: too few operands for instruction ^~~~~~ |
test/tools/llvm-mca/X86/Atom/resources-x86_64.s | ||
---|---|---|
230 | No, I mean the instruction implicitly uses the RAX register - use different registers to make the instruction more realistic. APM V3:
| |
232 | This triple is 64-bit - use 64-bit pointers (%rbx). | |
799 | This triple is 64-bit - use 64-bit pointers (%rbx). |
Don't use AL/AX/EAX/RAX as explicit ops - they are implicitly referenced by in the instruction