This is an archive of the discontinued LLVM Phabricator instance.

[X86] MCA tests for XCHG*, XADD* and CMPXCHG* instructions
ClosedPublic

Authored by avt77 on Jul 27 2018, 5:53 AM.

Details

Summary

Looking at D49861 Roman discovered that we don't have MCA tests for XCHG* instrs. This patch fixes this issue.

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Jul 27 2018, 5:53 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
676–677 ↗(On Diff #157674)

http://felixcloutier.com/x86/XCHG.html suggests that memory operand can be on either side,
so this needs to be repeated with swapped order.

avt77 added inline comments.Jul 27 2018, 6:27 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
676–677 ↗(On Diff #157674)

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?

lebedev.ri added inline comments.Jul 27 2018, 6:32 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
676–677 ↗(On Diff #157674)

In fact I checked it but decided not to include in the test because this test is not about syntax.

I agree. But the existing tests are doing exhaustive coverage - e.g. see sub, add.

Or you'd like to see it inside all 10 files?

All these generic files should be identical other than the CPU specified in run line.
One can argue that it is a bad design, to have to duplicate the tests for every CPU.
But this is what it is...

So yes, all of them, please.

avt77 updated this revision to Diff 157699.Jul 27 2018, 8:30 AM

I updated the tests accordingly to Roman's request.

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.

RKSimon added inline comments.Jul 30 2018, 4:36 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
674 ↗(On Diff #157699)

I don't think only byte ops have special case AL instructions? It's just ax/eax/rax according to APMv3

676–677 ↗(On Diff #157674)

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?

Please can you add XADD tests as well

RKSimon added inline comments.Aug 1 2018, 10:32 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
230 ↗(On Diff #157699)

Don't use AL/AX/EAX/RAX as explicit ops - they are implicitly referenced by in the instruction

233 ↗(On Diff #157699)

cmpxchg8b/cmpxchg16b tests were added at rL338595 (in their own file) - please remove them from this patch

676–677 ↗(On Diff #157674)

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.

craig.topper added inline comments.Aug 1 2018, 11:50 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
676–677 ↗(On Diff #157674)

Yeah its just a parser trick. The encoding is the same.

avt77 added inline comments.Aug 2 2018, 2:17 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
230 ↗(On Diff #157699)

It seems it deos not work:

<stdin>:225:1: error: too few operands for instruction
cmpxchgb %bl
^
<stdin>:226:10: error: invalid operand for instruction
cmpxchgb (%rbx)

^~~~~~

<stdin>:228:1: error: too few operands for instruction
cmpxchgw %bx
^
<stdin>:229:10: error: invalid operand for instruction
cmpxchgw (%rbx)

^~~~~~

<stdin>:231:1: error: too few operands for instruction
cmpxchgl %ebx
^
<stdin>:232:10: error: invalid operand for instruction
cmpxchgl (%ebx)

^~~~~~

<stdin>:234:1: error: too few operands for instruction
cmpxchgq %rbx
^
<stdin>:235:10: error: invalid operand for instruction
cmpxchgq (%rbx)

^~~~~~
avt77 updated this revision to Diff 158721.Aug 2 2018, 4:04 AM

The new tests were updated, xadd* tests were added.

RKSimon added inline comments.Aug 2 2018, 7:53 AM
test/tools/llvm-mca/X86/Atom/resources-x86_64.s
232 ↗(On Diff #158721)

This triple is 64-bit - use 64-bit pointers (%rbx).

799 ↗(On Diff #158721)

This triple is 64-bit - use 64-bit pointers (%rbx).

230 ↗(On Diff #157699)

No, I mean the instruction implicitly uses the RAX register - use different registers to make the instruction more realistic.

APM V3:

Compares the value in the AL, AX, EAX, or RAX register with the value in a register or a memory location (first operand). If the two values are equal, the instruction copies the value in the second operand to the first operand and sets the ZF flag in the rFLAGS register to 1. Otherwise, it copies the value in the first operand to the AL, AX, EAX, or RAX register and clears the ZF flag to 0.

avt77 updated this revision to Diff 159478.Aug 7 2018, 3:31 AM
avt77 retitled this revision from [X86] MCA tests for XCHG* and CMPXCHG* instructions to [X86] MCA tests for XCHG*, XADD* and CMPXCHG* instructions.

All last requirements were fixed.

RKSimon accepted this revision.Aug 7 2018, 4:32 AM

LGTM - thanks

This revision is now accepted and ready to land.Aug 7 2018, 4:32 AM
This revision was automatically updated to reflect the committed changes.