[PATCH] D49912: [X86] MCA tests for XCHG* and CMPXCHG* instructions

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: test/tools/llvm-mca/X86/Atom/resources-x86_64.s:676-677
+xchgb %bl, %cl
+xchgb %al, (%rbx)
+xchgb %bl, (%rbx)
+
----------------
RKSimon wrote:
> lebedev.ri wrote:
> > avt77 wrote:
> > > lebedev.ri wrote:
> > > > http://felixcloutier.com/x86/XCHG.html suggests that memory operand can be on either side,
> > > > so this needs to be repeated with swapped order.
> > > 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?
> > > 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.
> 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?
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.


================
Comment at: test/tools/llvm-mca/X86/Atom/resources-x86_64.s:230
+cmpxchgq %rax, %rbx
+cmpxchgq %rax, (%rbx)
+
----------------
Don't use AL/AX/EAX/RAX as explicit ops - they are implicitly referenced by in the instruction 


================
Comment at: test/tools/llvm-mca/X86/Atom/resources-x86_64.s:233
+cmpxchg8b (%rsi)
+cmpxchg16b (%rsi)
+
----------------
cmpxchg8b/cmpxchg16b tests were added at rL338595 (in their own file) - please remove them from this patch


https://reviews.llvm.org/D49912