[PATCH] x86: Add {load}/{store} tests for apx instructions.

[PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

gas/ChangeLog:

        * testsuite/gas/i386/x86-64.exp: Add {load}/{store} tests for apx
        instructions.
        * testsuite/gas/i386/x86-64-pseudos-apx.d: New test.
        * testsuite/gas/i386/x86-64-pseudos-apx.s: Ditto.
---
 gas/testsuite/gas/i386/x86-64-pseudos-apx.d | 165 ++++++++++++++++++++
 gas/testsuite/gas/i386/x86-64-pseudos-apx.s |  43 +++++
 gas/testsuite/gas/i386/x86-64.exp           |   1 +
 3 files changed, 209 insertions(+)
 create mode 100644 gas/testsuite/gas/i386/x86-64-pseudos-apx.d
 create mode 100644 gas/testsuite/gas/i386/x86-64-pseudos-apx.s

diff --git a/gas/testsuite/gas/i386/x86-64-pseudos-apx.d b/gas/testsuite/gas/i386/x86-64-pseudos-apx.d
new file mode 100644
index 00000000000..c2446351ecc
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-pseudos-apx.d
@@ -0,0 +1,165 @@
+#as:
+#objdump: -dw -Msuffix
+#name: APX x86-64 pseudo prefixes
+
+.*: +file format .*
+
+Disassembly of section \.text:
+
+0+ <_start>:
+[	 ]*[a-f0-9]+:[	 ]*d5 11 89 cf[	 ]+movl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 8b f9[	 ]+movl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 89 cf[	 ]+movl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 8b 39[	 ]+movl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 89 39[	 ]+movl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 11 cf[	 ]+adcl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 13 f9[	 ]+adcl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 11 cf[	 ]+adcl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 13 39[	 ]+adcl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 11 39[	 ]+adcl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 01 cf[	 ]+addl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 03 f9[	 ]+addl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 01 cf[	 ]+addl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 03 39[	 ]+addl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 01 39[	 ]+addl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 21 cf[	 ]+andl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 23 f9[	 ]+andl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 21 cf[	 ]+andl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 23 39[	 ]+andl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 21 39[	 ]+andl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 39 cf[	 ]+cmpl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 3b f9[	 ]+cmpl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 39 cf[	 ]+cmpl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 3b 39[	 ]+cmpl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 39 39[	 ]+cmpl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 09 cf[	 ]+orl    %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 0b f9[	 ]+orl.s  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 09 cf[	 ]+orl    %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 0b 39[	 ]+orl    \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 09 39[	 ]+orl    %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 19 cf[	 ]+sbbl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 1b f9[	 ]+sbbl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 19 cf[	 ]+sbbl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 1b 39[	 ]+sbbl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 19 39[	 ]+sbbl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 29 cf[	 ]+subl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 2b f9[	 ]+subl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 29 cf[	 ]+subl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 2b 39[	 ]+subl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 29 39[	 ]+subl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 85 cf[	 ]+testl  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 85 f9[	 ]+testl  %r31d,%ecx
+[	 ]*[a-f0-9]+:[	 ]*d5 11 85 cf[	 ]+testl  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 85 39[	 ]+testl  %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 85 39[	 ]+testl  %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 31 cf[	 ]+xorl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 33 f9[	 ]+xorl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 11 31 cf[	 ]+xorl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 33 39[	 ]+xorl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 31 39[	 ]+xorl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 11 87 cf[	 ]+xchgl  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*d5 44 87 f9[	 ]+xchgl  %r31d,%ecx
+[	 ]*[a-f0-9]+:[	 ]*d5 11 87 cf[	 ]+xchgl  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 87 39[	 ]+xchgl  %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*67 d5 44 87 39[	 ]+xchgl  %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*d5 91 28 17[	 ]+movaps \(%r31\),%xmm2
+[	 ]*[a-f0-9]+:[	 ]*d5 91 28 17[	 ]+movaps \(%r31\),%xmm2
+[	 ]*[a-f0-9]+:[	 ]*d5 91 29 17[	 ]+movaps %xmm2,\(%r31\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 11 cf[	 ]+adcl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 13 f9[	 ]+adcl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 11 cf[	 ]+adcl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 13 39[	 ]+adcl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 11 39[	 ]+adcl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 01 cf[	 ]+addl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 03 f9[	 ]+addl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 01 cf[	 ]+addl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 03 39[	 ]+addl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 01 39[	 ]+addl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 21 cf[	 ]+andl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 23 f9[	 ]+andl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 21 cf[	 ]+andl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 23 39[	 ]+andl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 21 39[	 ]+andl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 09 cf[	 ]+orl    %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 0b f9[	 ]+orl.s  %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 09 cf[	 ]+orl    %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 0b 39[	 ]+orl    \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 09 39[	 ]+orl    %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 19 cf[	 ]+sbbl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 1b f9[	 ]+sbbl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 19 cf[	 ]+sbbl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 1b 39[	 ]+sbbl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 19 39[	 ]+sbbl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 29 cf[	 ]+subl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 2b f9[	 ]+subl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 29 cf[	 ]+subl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 2b 39[	 ]+subl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 29 39[	 ]+subl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 31 cf[	 ]+xorl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 08 33 f9[	 ]+xorl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 08 31 cf[	 ]+xorl   %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 33 39[	 ]+xorl   \(%ecx\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 64 7c 08 31 39[	 ]+xorl   %r31d,\(%ecx\)
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 03 f9[	 ]+addl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 01 cf[	 ]+addl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 01 38[	 ]+addq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 03 38[	 ]+addq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 01 38[	 ]+addq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 03 38[	 ]+addq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 0b f9[	 ]+orl.s  %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 09 cf[	 ]+orl    %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 09 38[	 ]+orq    %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 0b 38[	 ]+orq    \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 09 38[	 ]+orq    %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 0b 38[	 ]+orq    \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 13 f9[	 ]+adcl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 11 cf[	 ]+adcl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 11 38[	 ]+adcq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 13 38[	 ]+adcq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 11 38[	 ]+adcq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 13 38[	 ]+adcq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 1b f9[	 ]+sbbl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 19 cf[	 ]+sbbl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 19 38[	 ]+sbbq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 1b 38[	 ]+sbbq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 19 38[	 ]+sbbq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 1b 38[	 ]+sbbq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 23 f9[	 ]+andl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 21 cf[	 ]+andl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 21 38[	 ]+andq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 23 38[	 ]+andq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 21 38[	 ]+andq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 23 38[	 ]+andq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 2b f9[	 ]+subl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 29 cf[	 ]+subl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 29 38[	 ]+subq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 2b 38[	 ]+subq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 29 38[	 ]+subq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 2b 38[	 ]+subq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 18 33 f9[	 ]+xorl.s %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 18 31 cf[	 ]+xorl   %ecx,%r31d,%eax
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 31 38[	 ]+xorq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 33 38[	 ]+xorq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 31 38[	 ]+xorq   %r31,\(%r8\),%r16
+[	 ]*[a-f0-9]+:[	 ]*62 44 fc 10 33 38[	 ]+xorq   \(%r8\),%r31,%r16
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 0c 03 f9[	 ]+\{nf\} addl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 0c 01 cf[	 ]+\{nf\} addl %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 03 38[	 ]+\{nf\} addl \(%r8d\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 01 38[	 ]+\{nf\} addl %r31d,\(%r8d\)
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 0c 0b f9[	 ]+\{nf\} orl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 0c 09 cf[	 ]+\{nf\} orl %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 0b 38[	 ]+\{nf\} orl \(%r8d\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 09 38[	 ]+\{nf\} orl %r31d,\(%r8d\)
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 0c 23 f9[	 ]+\{nf\} andl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 0c 21 cf[	 ]+\{nf\} andl %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 23 38[	 ]+\{nf\} andl \(%r8d\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 21 38[	 ]+\{nf\} andl %r31d,\(%r8d\)
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 0c 2b f9[	 ]+\{nf\} subl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 0c 29 cf[	 ]+\{nf\} subl %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 2b 38[	 ]+\{nf\} subl \(%r8d\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 29 38[	 ]+\{nf\} subl %r31d,\(%r8d\)
+[	 ]*[a-f0-9]+:[	 ]*62 64 7c 0c 33 f9[	 ]+\{nf\} xorl.s %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*62 dc 7c 0c 31 cf[	 ]+\{nf\} xorl %ecx,%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 33 38[	 ]+\{nf\} xorl \(%r8d\),%r31d
+[	 ]*[a-f0-9]+:[	 ]*67 62 44 7c 0c 31 38[	 ]+\{nf\} xorl %r31d,\(%r8d\)
+#pass
diff --git a/gas/testsuite/gas/i386/x86-64-pseudos-apx.s b/gas/testsuite/gas/i386/x86-64-pseudos-apx.s
new file mode 100644
index 00000000000..3532cb77e7f
--- /dev/null
+++ b/gas/testsuite/gas/i386/x86-64-pseudos-apx.s
@@ -0,0 +1,43 @@
+# Check 64bit APX instructions with pseudo prefixes for encoding
+
+.text
+_start:
+        #APX REX2
+        .irp m, mov, adc, add, and, cmp, or, sbb, sub, test, xor, xchg
+        \m %ecx, %r31d
+        {load}  \m %ecx, %r31d
+        {store} \m %ecx, %r31d
+        {load}  \m (%ecx), %r31d
+        {store} \m %r31d, (%ecx)
+        .endr
+
+        movaps (%r31),%xmm2
+        {load} movaps (%r31),%xmm2
+        {store} movaps %xmm2, (%r31)
+
+        #APX EVEX promoted from legacy
+        .irp m, adc, add, and, or, sbb, sub, xor
+        {evex}         \m %ecx, %r31d
+        {evex} {load}  \m %ecx, %r31d
+        {evex} {store} \m %ecx, %r31d
+        {evex} {load}  \m (%ecx), %r31d
+        {evex} {store} \m %r31d, (%ecx)
+        .endr
+
+        #APX NDD
+        .irp m, add, or, adc, sbb, and, sub, xor
+        {load}  \m %ecx, %r31d, %eax
+        {store} \m %ecx, %r31d, %eax
+        {load}  \m %r31,(%r8),%r16
+        {load}  \m (%r8),%r31,%r16
+        {store} \m %r31,(%r8),%r16
+        {store} \m (%r8),%r31,%r16
+        .endr
+
+        #APX NF
+        .irp m, add, or, and, sub, xor
+        {nf} {load}  \m %ecx, %r31d
+        {nf} {store} \m %ecx, %r31d
+        {nf} {load}  \m (%r8d), %r31d
+        {nf} {store} \m %r31d, (%r8d)
+        .endr
diff --git a/gas/testsuite/gas/i386/x86-64.exp b/gas/testsuite/gas/i386/x86-64.exp
index a8d49cefe8c..407ea85ae11 100644
--- a/gas/testsuite/gas/i386/x86-64.exp
+++ b/gas/testsuite/gas/i386/x86-64.exp
@@ -555,6 +555,7 @@ run_dump_test "x86-64-cet-intel"
 run_list_test "x86-64-cet-ibt-inval"
 run_list_test "x86-64-cet-shstk-inval"
 run_dump_test "x86-64-pseudos"
+run_dump_test "x86-64-pseudos-apx"
 run_list_test "x86-64-pseudos-bad"
 run_list_test "x86-64-inval-pseudo" "-al"
 run_dump_test "x86-64-notrack"
-- 
2.34.1

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 26.06.2024 13:26, Cui, Lili wrote:
> gas/ChangeLog:
> 
>         * testsuite/gas/i386/x86-64.exp: Add {load}/{store} tests for apx
>         instructions.
>         * testsuite/gas/i386/x86-64-pseudos-apx.d: New test.
>         * testsuite/gas/i386/x86-64-pseudos-apx.s: Ditto.

In principle I wouldn't mind if this went in as is. Nevertheless, a few
comments / possible requests:

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-pseudos-apx.s
> @@ -0,0 +1,43 @@
> +# Check 64bit APX instructions with pseudo prefixes for encoding
> +
> +.text
> +_start:
> +        #APX REX2
> +        .irp m, mov, adc, add, and, cmp, or, sbb, sub, test, xor, xchg
> +        \m %ecx, %r31d
> +        {load}  \m %ecx, %r31d
> +        {store} \m %ecx, %r31d
> +        {load}  \m (%ecx), %r31d
> +        {store} \m %r31d, (%ecx)
> +        .endr
> +
> +        movaps (%r31),%xmm2
> +        {load} movaps (%r31),%xmm2
> +        {store} movaps %xmm2, (%r31)
> +
> +        #APX EVEX promoted from legacy
> +        .irp m, adc, add, and, or, sbb, sub, xor
> +        {evex}         \m %ecx, %r31d
> +        {evex} {load}  \m %ecx, %r31d
> +        {evex} {store} \m %ecx, %r31d
> +        {evex} {load}  \m (%ecx), %r31d
> +        {evex} {store} \m %r31d, (%ecx)
> +        .endr

Like you have it here, imo the other sets would also benefit from having
an instance with neither {load} nor {store}.

> +        #APX NDD
> +        .irp m, add, or, adc, sbb, and, sub, xor
> +        {load}  \m %ecx, %r31d, %eax
> +        {store} \m %ecx, %r31d, %eax
> +        {load}  \m %r31,(%r8),%r16
> +        {load}  \m (%r8),%r31,%r16
> +        {store} \m %r31,(%r8),%r16
> +        {store} \m (%r8),%r31,%r16
> +        .endr
> +
> +        #APX NF
> +        .irp m, add, or, and, sub, xor
> +        {nf} {load}  \m %ecx, %r31d
> +        {nf} {store} \m %ecx, %r31d
> +        {nf} {load}  \m (%r8d), %r31d
> +        {nf} {store} \m %r31d, (%r8d)
> +        .endr

I'm not convinced this last set is particularly useful. If it's being
kept, question would be why NF and NDD together aren't tested.

Having reached the end of the file: What about CTESTcc and CCMPcc?

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-pseudos-apx.s
> > @@ -0,0 +1,43 @@
> > +# Check 64bit APX instructions with pseudo prefixes for encoding
> > +
> > +.text
> > +_start:
> > +        #APX REX2
> > +        .irp m, mov, adc, add, and, cmp, or, sbb, sub, test, xor, xchg
> > +        \m %ecx, %r31d
> > +        {load}  \m %ecx, %r31d
> > +        {store} \m %ecx, %r31d
> > +        {load}  \m (%ecx), %r31d
> > +        {store} \m %r31d, (%ecx)
> > +        .endr
> > +
> > +        movaps (%r31),%xmm2
> > +        {load} movaps (%r31),%xmm2
> > +        {store} movaps %xmm2, (%r31)
> > +
> > +        #APX EVEX promoted from legacy
> > +        .irp m, adc, add, and, or, sbb, sub, xor
> > +        {evex}         \m %ecx, %r31d
> > +        {evex} {load}  \m %ecx, %r31d
> > +        {evex} {store} \m %ecx, %r31d
> > +        {evex} {load}  \m (%ecx), %r31d
> > +        {evex} {store} \m %r31d, (%ecx)
> > +        .endr
> 
> Like you have it here, imo the other sets would also benefit from having an
> instance with neither {load} nor {store}.
> 

Done.

> > +        #APX NDD
> > +        .irp m, add, or, adc, sbb, and, sub, xor
> > +        {load}  \m %ecx, %r31d, %eax
> > +        {store} \m %ecx, %r31d, %eax
> > +        {load}  \m %r31,(%r8),%r16
> > +        {load}  \m (%r8),%r31,%r16
> > +        {store} \m %r31,(%r8),%r16
> > +        {store} \m (%r8),%r31,%r16
> > +        .endr
> > +
> > +        #APX NF
> > +        .irp m, add, or, and, sub, xor
> > +        {nf} {load}  \m %ecx, %r31d
> > +        {nf} {store} \m %ecx, %r31d
> > +        {nf} {load}  \m (%r8d), %r31d
> > +        {nf} {store} \m %r31d, (%r8d)
> > +        .endr
> 
> I'm not convinced this last set is particularly useful. If it's being kept, question
> would be why NF and NDD together aren't tested.
> 
> Having reached the end of the file: What about CTESTcc and CCMPcc?
> 

I met some issues when adding CTESTcc and CCMPcc. 

For example, the disassembler for "{load} ccmpbl %edx, %eax" now looks a bit weird. 

"ccmpbl {dfv=}.s %edx,%eax"

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 01.07.2024 15:42, Cui, Lili wrote:
>> Having reached the end of the file: What about CTESTcc and CCMPcc?
>>
> 
> I met some issues when adding CTESTcc and CCMPcc. 
> 
> For example, the disassembler for "{load} ccmpbl %edx, %eax" now looks a bit weird. 
> 
> "ccmpbl {dfv=}.s %edx,%eax"

Well ... Something clearly needs doing about this. I'm now even more
convinced we want tests for these.

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> On 01.07.2024 15:42, Cui, Lili wrote:
> >> Having reached the end of the file: What about CTESTcc and CCMPcc?
> >>
> >

When adding swap test cases for CTESTcc, I found that it does not support swapping operands, but test, {evex} test and ctest template insns all have D, which does not match the SDM/APX spec, I want to remove D for them. I'm a bit unsure if there's any particular reason why the legacy test added D ?

test, 0x84, 0, D|W|C|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
test, 0x840a, 0, D|W|C|CheckOperandSize|Modrm|EVexMap4|Scc|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
ctest<scc>, 0x840<scc:opc>, APX_F, D|W|C|CheckOperandSize|Modrm|EVexMap4|Scc|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

> > I met some issues when adding CTESTcc and CCMPcc.
> >
> > For example, the disassembler for "{load} ccmpbl %edx, %eax" now looks a bit
> weird.
> >
> > "ccmpbl {dfv=}.s %edx,%eax"
> 
> Well ... Something clearly needs doing about this. I'm now even more convinced
> we want tests for these.
> 
Indeed, I added a macro %SW to indicate operands were swapped when suffix_always is true.  Are you ok with it?

--- a/opcodes/i386-dis-evex.h
+++ b/opcodes/i386-dis-evex.h
@@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
     /* 38 */
     { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
     { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
-    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
-    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
+    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
+    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
     { Bad_Opcode },
     { Bad_Opcode },
     { Bad_Opcode },
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 978440fa3f1..483cc16ff16 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -1809,6 +1809,8 @@ struct dis386 {
           in MAP4.
    "ZU" => print 'zu' if EVEX.ZU=1.
    "SC" => print suffix SCC for SCC insns
+   "SW" => print '.s' to indicate operands were swapped when suffix_always is
+          true.
    "YK" keep unused, to avoid ambiguity with the combined use of Y and K.
    "YX" keep unused, to avoid ambiguity with the combined use of Y and X.
    "LQ" => print 'l' ('d' in Intel mode) or 'q' for memory operand, cond
@@ -10927,6 +10929,14 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
                *ins->obufp++ = ins->vex.w ? 'd': 's';
              else if (last[0] == 'B')
                *ins->obufp++ = ins->vex.w ? 'w': 'b';
+             else if (last[0] == 'S')
+               {
+                 if (sizeflag & SUFFIX_ALWAYS)
+                   {
+                     *ins->obufp++ = '.';
+                     *ins->obufp++ = 's';
+                   }
+               }

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 02.07.2024 09:09, Cui, Lili wrote:
>> On 01.07.2024 15:42, Cui, Lili wrote:
>>>> Having reached the end of the file: What about CTESTcc and CCMPcc?
>>>>
>>>
> 
> When adding swap test cases for CTESTcc, I found that it does not support swapping operands, but test, {evex} test and ctest template insns all have D, which does not match the SDM/APX spec, I want to remove D for them. I'm a bit unsure if there's any particular reason why the legacy test added D ?

Did you look at the commit doing so?

> test, 0x84, 0, D|W|C|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> test, 0x840a, 0, D|W|C|CheckOperandSize|Modrm|EVexMap4|Scc|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> ctest<scc>, 0x840<scc:opc>, APX_F, D|W|C|CheckOperandSize|Modrm|EVexMap4|Scc|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

If you remove D, I expect "ctestb (%rax),%ecx" and "{evex} test (%rax),%ecx"
would cease working (assuming they do work right now; if they don't, that's
yet another thing in need of fixing).

>>> I met some issues when adding CTESTcc and CCMPcc.
>>>
>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now looks a bit
>> weird.
>>>
>>> "ccmpbl {dfv=}.s %edx,%eax"
>>
>> Well ... Something clearly needs doing about this. I'm now even more convinced
>> we want tests for these.
>>
> Indeed, I added a macro %SW to indicate operands were swapped when suffix_always is true.  Are you ok with it?

In principle (and if then used consistently), why not. However, as long
as you use it only for ...

> --- a/opcodes/i386-dis-evex.h
> +++ b/opcodes/i386-dis-evex.h
> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
>      /* 38 */
>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },

... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> >>>> Having reached the end of the file: What about CTESTcc and CCMPcc?
> >>>>
> >>>
> >
> > When adding swap test cases for CTESTcc, I found that it does not support
> swapping operands, but test, {evex} test and ctest template insns all have D,
> which does not match the SDM/APX spec, I want to remove D for them. I'm a bit
> unsure if there's any particular reason why the legacy test added D ?
> 
> Did you look at the commit doing so?
> 

Ok, I found the commit this time, but I'm even more confused because I didn't find the basis for the second test from SDM.  

-test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseIndex }
-test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, { Byte|Word|Dword|Qword|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64 }
+test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 02.07.2024 12:29, Cui, Lili wrote:
>>>>>> Having reached the end of the file: What about CTESTcc and CCMPcc?
>>>>>>
>>>>>
>>>
>>> When adding swap test cases for CTESTcc, I found that it does not support
>> swapping operands, but test, {evex} test and ctest template insns all have D,
>> which does not match the SDM/APX spec, I want to remove D for them. I'm a bit
>> unsure if there's any particular reason why the legacy test added D ?
>>
>> Did you look at the commit doing so?
>>
> 
> Ok, I found the commit this time, but I'm even more confused because I didn't find the basis for the second test from SDM.  
> 
> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseIndex }
> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, { Byte|Word|Dword|Qword|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64 }
> +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

"basis for the second test"? What do you mean? Are you referring to the SDM spelling
out only one operand order? That's surely a (very minor) shortcoming there. TEST is
commutative, and hence whatever order the programmer likes should be accepted by an
assembler.

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, July 2, 2024 8:15 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: binutils@sourceware.org; hjl.tools@gmail.com
> Subject: Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.
> 
> On 02.07.2024 12:29, Cui, Lili wrote:
> >>>>>> Having reached the end of the file: What about CTESTcc and CCMPcc?
> >>>>>>
> >>>>>
> >>>
> >>> When adding swap test cases for CTESTcc, I found that it does not
> >>> support
> >> swapping operands, but test, {evex} test and ctest template insns all
> >> have D, which does not match the SDM/APX spec, I want to remove D for
> >> them. I'm a bit unsure if there's any particular reason why the legacy test
> added D ?
> >>
> >> Did you look at the commit doing so?
> >>
> >
> > Ok, I found the commit this time, but I'm even more confused because I
> didn't find the basis for the second test from SDM.
> >
> > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > Reg8|Reg16|Reg32|Reg64,
> >
> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
> x }
> > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
> > +Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> > +}
> 
> "basis for the second test"? What do you mean? Are you referring to the SDM
> spelling out only one operand order? That's surely a (very minor) shortcoming
> there. TEST is commutative, and hence whatever order the programmer likes
> should be accepted by an assembler.

Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx), %ax" in the SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is supported.  Anyway, their assemblers are same.
I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in assembler, currently "ctestb(%rax),%ecx" does not work, I need to fix it first.


Thanks,
Lili.

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> > On 02.07.2024 12:29, Cui, Lili wrote:
> > >>>>>> Having reached the end of the file: What about CTESTcc and
> CCMPcc?
> > >>>>>>
> > >>>>>
> > >>>
> > >>> When adding swap test cases for CTESTcc, I found that it does not
> > >>> support
> > >> swapping operands, but test, {evex} test and ctest template insns
> > >> all have D, which does not match the SDM/APX spec, I want to remove
> > >> D for them. I'm a bit unsure if there's any particular reason why
> > >> the legacy test
> > added D ?
> > >>
> > >> Did you look at the commit doing so?
> > >>
> > >
> > > Ok, I found the commit this time, but I'm even more confused because
> > > I
> > didn't find the basis for the second test from SDM.
> > >
> > > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > > Reg8|Reg16|Reg32|Reg64,
> > >
> >
> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
> > x }
> > > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > > Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> > Reg8|Reg16|Reg32|Reg64 }
> > > +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
> > > +Reg8|Reg16|Reg32|Reg64,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> > > +}
> >
> > "basis for the second test"? What do you mean? Are you referring to
> > the SDM spelling out only one operand order? That's surely a (very
> > minor) shortcoming there. TEST is commutative, and hence whatever
> > order the programmer likes should be accepted by an assembler.
> 
> Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx), %ax" in the
> SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is supported.
> Anyway, their assemblers are same.
> I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in assembler,
> currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
> 
$ cat test.s
test (%edx), %eax
test %eax, (%edx)

$ as --64  test.s -o test.o
$ objdump  -dw -Msuffix test.o
0000000000000000 <_start>:
   0:   67 85 02                testl  %eax,(%edx)
   3:   67 85 02                testl  %eax,(%edx)

Hi HJ,

Do you remember why we supported "test (%edx), %eax" in the original version? I'm curious about it. 

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

On Wed, Jul 3, 2024, 10:41 AM Cui, Lili <lili.cui@intel.com> wrote:

> > > On 02.07.2024 12:29, Cui, Lili wrote:
> > > >>>>>> Having reached the end of the file: What about CTESTcc and
> > CCMPcc?
> > > >>>>>>
> > > >>>>>
> > > >>>
> > > >>> When adding swap test cases for CTESTcc, I found that it does not
> > > >>> support
> > > >> swapping operands, but test, {evex} test and ctest template insns
> > > >> all have D, which does not match the SDM/APX spec, I want to remove
> > > >> D for them. I'm a bit unsure if there's any particular reason why
> > > >> the legacy test
> > > added D ?
> > > >>
> > > >> Did you look at the commit doing so?
> > > >>
> > > >
> > > > Ok, I found the commit this time, but I'm even more confused because
> > > > I
> > > didn't find the basis for the second test from SDM.
> > > >
> > > > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > > > Reg8|Reg16|Reg32|Reg64,
> > > >
> > >
> > Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
> > > x }
> > > > -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> > > > Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> > > Reg8|Reg16|Reg32|Reg64 }
> > > > +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
> > > > +Reg8|Reg16|Reg32|Reg64,
> > > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> > > > +}
> > >
> > > "basis for the second test"? What do you mean? Are you referring to
> > > the SDM spelling out only one operand order? That's surely a (very
> > > minor) shortcoming there. TEST is commutative, and hence whatever
> > > order the programmer likes should be accepted by an assembler.
> >
> > Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx), %ax" in
> the
> > SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is
> supported.
> > Anyway, their assemblers are same.
> > I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in
> assembler,
> > currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
> >
> $ cat test.s
> test (%edx), %eax
> test %eax, (%edx)
>
> $ as --64  test.s -o test.o
> $ objdump  -dw -Msuffix test.o
> 0000000000000000 <_start>:
>    0:   67 85 02                testl  %eax,(%edx)
>    3:   67 85 02                testl  %eax,(%edx)
>
> Hi HJ,
>
> Do you remember why we supported "test (%edx), %eax" in the original
> version? I'm curious about it.


I don't remember why.  I think we should follow SDM.


>
> Thanks,
> Lili.
>

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 03.07.2024 06:01, H.J. Lu wrote:
> On Wed, Jul 3, 2024, 10:41 AM Cui, Lili <lili.cui@intel.com> wrote:
> 
>>>> On 02.07.2024 12:29, Cui, Lili wrote:
>>>>>>>>>> Having reached the end of the file: What about CTESTcc and
>>> CCMPcc?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>> When adding swap test cases for CTESTcc, I found that it does not
>>>>>>> support
>>>>>> swapping operands, but test, {evex} test and ctest template insns
>>>>>> all have D, which does not match the SDM/APX spec, I want to remove
>>>>>> D for them. I'm a bit unsure if there's any particular reason why
>>>>>> the legacy test
>>>> added D ?
>>>>>>
>>>>>> Did you look at the commit doing so?
>>>>>>
>>>>>
>>>>> Ok, I found the commit this time, but I'm even more confused because
>>>>> I
>>>> didn't find the basis for the second test from SDM.
>>>>>
>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
>>>>> Reg8|Reg16|Reg32|Reg64,
>>>>>
>>>>
>>> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
>>>> x }
>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
>>>>> Byte|Word|Dword|Qword|Unspecified|BaseIndex,
>>>> Reg8|Reg16|Reg32|Reg64 }
>>>>> +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
>>>>> +Reg8|Reg16|Reg32|Reg64,
>>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
>>>>> +}
>>>>
>>>> "basis for the second test"? What do you mean? Are you referring to
>>>> the SDM spelling out only one operand order? That's surely a (very
>>>> minor) shortcoming there. TEST is commutative, and hence whatever
>>>> order the programmer likes should be accepted by an assembler.
>>>
>>> Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx), %ax" in
>> the
>>> SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is
>> supported.
>>> Anyway, their assemblers are same.
>>> I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in
>> assembler,
>>> currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
>>>
>> $ cat test.s
>> test (%edx), %eax
>> test %eax, (%edx)
>>
>> $ as --64  test.s -o test.o
>> $ objdump  -dw -Msuffix test.o
>> 0000000000000000 <_start>:
>>    0:   67 85 02                testl  %eax,(%edx)
>>    3:   67 85 02                testl  %eax,(%edx)
>>
>> Hi HJ,
>>
>> Do you remember why we supported "test (%edx), %eax" in the original
>> version? I'm curious about it.
> 
> I don't remember why.  I think we should follow SDM.

We should follow what makes sense in written code. Other assemblers
permit either order of the operands, when their order doesn't matter.
So should gas do. The fact that the SDM spells out only one order of
operands is (presumably) connected to there being only one encoding
(unlike for e.g. CMP). This does not mean we should be unfriendly to
people and force a purely stylistic restriction onto them. And just
to mention it: For TEST both forms were supported virtually forever.
Quite likely simply because, as said, other assemblers to do, too.

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> On 03.07.2024 06:01, H.J. Lu wrote:
> > On Wed, Jul 3, 2024, 10:41 AM Cui, Lili <lili.cui@intel.com> wrote:
> >
> >>>> On 02.07.2024 12:29, Cui, Lili wrote:
> >>>>>>>>>> Having reached the end of the file: What about CTESTcc and
> >>> CCMPcc?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>> When adding swap test cases for CTESTcc, I found that it does
> >>>>>>> not support
> >>>>>> swapping operands, but test, {evex} test and ctest template insns
> >>>>>> all have D, which does not match the SDM/APX spec, I want to
> >>>>>> remove D for them. I'm a bit unsure if there's any particular
> >>>>>> reason why the legacy test
> >>>> added D ?
> >>>>>>
> >>>>>> Did you look at the commit doing so?
> >>>>>>
> >>>>>
> >>>>> Ok, I found the commit this time, but I'm even more confused
> >>>>> because I
> >>>> didn't find the basis for the second test from SDM.
> >>>>>
> >>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> >>>>> Reg8|Reg16|Reg32|Reg64,
> >>>>>
> >>>>
> >>>
> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
> >>>> x }
> >>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> >>>>> Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> >>>> Reg8|Reg16|Reg32|Reg64 }
> >>>>> +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
> >>>>> +Reg8|Reg16|Reg32|Reg64,
> >>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> >>>>> +}
> >>>>
> >>>> "basis for the second test"? What do you mean? Are you referring to
> >>>> the SDM spelling out only one operand order? That's surely a (very
> >>>> minor) shortcoming there. TEST is commutative, and hence whatever
> >>>> order the programmer likes should be accepted by an assembler.
> >>>
> >>> Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx),
> >>> %ax" in
> >> the
> >>> SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is
> >> supported.
> >>> Anyway, their assemblers are same.
> >>> I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in
> >> assembler,
> >>> currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
> >>>
> >> $ cat test.s
> >> test (%edx), %eax
> >> test %eax, (%edx)
> >>
> >> $ as --64  test.s -o test.o
> >> $ objdump  -dw -Msuffix test.o
> >> 0000000000000000 <_start>:
> >>    0:   67 85 02                testl  %eax,(%edx)
> >>    3:   67 85 02                testl  %eax,(%edx)
> >>
> >> Hi HJ,
> >>
> >> Do you remember why we supported "test (%edx), %eax" in the original
> >> version? I'm curious about it.
> >
> > I don't remember why.  I think we should follow SDM.
> 
> We should follow what makes sense in written code. Other assemblers permit
> either order of the operands, when their order doesn't matter.
> So should gas do. The fact that the SDM spells out only one order of operands
> is (presumably) connected to there being only one encoding (unlike for e.g.
> CMP). This does not mean we should be unfriendly to people and force a
> purely stylistic restriction onto them. And just to mention it: For TEST both
> forms were supported virtually forever.
> Quite likely simply because, as said, other assemblers to do, too.
> 

Yes, I found that LLVM and NSAM also support "test (%edx), %eax", and llvm has just created a jira to follow our decision (They also want to remove it instead of continuing to make similar extensions to ctest, but finally it depends on us).

Personally, since no one knows the original intention of this alias, it seems more like a bug to me, because I can't see its benefit, SDM clearly says that "When two operands are present in an arithmetic or logical instruction, the right operand is the source and the left operand is the destination". "test eax, [edx]" is very confusing to me. Someone writing like this would mistakenly think that dest is a register, but in fact it is a memory. I think this is a trap, we should report an error for it.

If you insist on not deleting "test (%edx), %eax",  and extending it to ctest, I will also agree. I don't think anyone would use it that way.

Thanks,
Lili.









 

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 03.07.2024 14:17, Cui, Lili wrote:
>> On 03.07.2024 06:01, H.J. Lu wrote:
>>> On Wed, Jul 3, 2024, 10:41 AM Cui, Lili <lili.cui@intel.com> wrote:
>>>
>>>>>> On 02.07.2024 12:29, Cui, Lili wrote:
>>>>>>>>>>>> Having reached the end of the file: What about CTESTcc and
>>>>> CCMPcc?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> When adding swap test cases for CTESTcc, I found that it does
>>>>>>>>> not support
>>>>>>>> swapping operands, but test, {evex} test and ctest template insns
>>>>>>>> all have D, which does not match the SDM/APX spec, I want to
>>>>>>>> remove D for them. I'm a bit unsure if there's any particular
>>>>>>>> reason why the legacy test
>>>>>> added D ?
>>>>>>>>
>>>>>>>> Did you look at the commit doing so?
>>>>>>>>
>>>>>>>
>>>>>>> Ok, I found the commit this time, but I'm even more confused
>>>>>>> because I
>>>>>> didn't find the basis for the second test from SDM.
>>>>>>>
>>>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
>>>>>>> Reg8|Reg16|Reg32|Reg64,
>>>>>>>
>>>>>>
>>>>>
>> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
>>>>>> x }
>>>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
>>>>>>> Byte|Word|Dword|Qword|Unspecified|BaseIndex,
>>>>>> Reg8|Reg16|Reg32|Reg64 }
>>>>>>> +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
>>>>>>> +Reg8|Reg16|Reg32|Reg64,
>>>>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
>>>>>>> +}
>>>>>>
>>>>>> "basis for the second test"? What do you mean? Are you referring to
>>>>>> the SDM spelling out only one operand order? That's surely a (very
>>>>>> minor) shortcoming there. TEST is commutative, and hence whatever
>>>>>> order the programmer likes should be accepted by an assembler.
>>>>>
>>>>> Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx),
>>>>> %ax" in
>>>> the
>>>>> SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is
>>>> supported.
>>>>> Anyway, their assemblers are same.
>>>>> I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in
>>>> assembler,
>>>>> currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
>>>>>
>>>> $ cat test.s
>>>> test (%edx), %eax
>>>> test %eax, (%edx)
>>>>
>>>> $ as --64  test.s -o test.o
>>>> $ objdump  -dw -Msuffix test.o
>>>> 0000000000000000 <_start>:
>>>>    0:   67 85 02                testl  %eax,(%edx)
>>>>    3:   67 85 02                testl  %eax,(%edx)
>>>>
>>>> Hi HJ,
>>>>
>>>> Do you remember why we supported "test (%edx), %eax" in the original
>>>> version? I'm curious about it.
>>>
>>> I don't remember why.  I think we should follow SDM.
>>
>> We should follow what makes sense in written code. Other assemblers permit
>> either order of the operands, when their order doesn't matter.
>> So should gas do. The fact that the SDM spells out only one order of operands
>> is (presumably) connected to there being only one encoding (unlike for e.g.
>> CMP). This does not mean we should be unfriendly to people and force a
>> purely stylistic restriction onto them. And just to mention it: For TEST both
>> forms were supported virtually forever.
>> Quite likely simply because, as said, other assemblers to do, too.
>>
> 
> Yes, I found that LLVM and NSAM also support "test (%edx), %eax", and llvm has just created a jira to follow our decision (They also want to remove it instead of continuing to make similar extensions to ctest, but finally it depends on us).
> 
> Personally, since no one knows the original intention of this alias, it seems more like a bug to me, because I can't see its benefit, SDM clearly says that "When two operands are present in an arithmetic or logical instruction, the right operand is the source and the left operand is the destination". "test eax, [edx]" is very confusing to me. Someone writing like this would mistakenly think that dest is a register, but in fact it is a memory. I think this is a trap, we should report an error for it.

The thing you're apparently missing is that TEST (like CMP) has no destination.
It has two sources instead.

> If you insist on not deleting "test (%edx), %eax",  and extending it to ctest, I will also agree. I don't think anyone would use it that way.

I definitely want the alternative form to be kept, and functioning.

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> >>>>>>>>>>>> Having reached the end of the file: What about CTESTcc and
> >>>>> CCMPcc?
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> When adding swap test cases for CTESTcc, I found that it does
> >>>>>>>>> not support
> >>>>>>>> swapping operands, but test, {evex} test and ctest template
> >>>>>>>> insns all have D, which does not match the SDM/APX spec, I want
> >>>>>>>> to remove D for them. I'm a bit unsure if there's any
> >>>>>>>> particular reason why the legacy test
> >>>>>> added D ?
> >>>>>>>>
> >>>>>>>> Did you look at the commit doing so?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Ok, I found the commit this time, but I'm even more confused
> >>>>>>> because I
> >>>>>> didn't find the basis for the second test from SDM.
> >>>>>>>
> >>>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> >>>>>>> Reg8|Reg16|Reg32|Reg64,
> >>>>>>>
> >>>>>>
> >>>>>
> >>
> Reg8|Reg16|Reg32|Reg64|Unspecified|Byte|Word|Dword|Qword|BaseInde
> >>>>>> x }
> >>>>>>> -test, 0x84, None, 0, W|CheckRegSize|Modrm|No_sSuf, {
> >>>>>>> Byte|Word|Dword|Qword|Unspecified|BaseIndex,
> >>>>>> Reg8|Reg16|Reg32|Reg64 }
> >>>>>>> +test, 0x84, None, 0, D|W|C|CheckRegSize|Modrm|No_sSuf, {
> >>>>>>> +Reg8|Reg16|Reg32|Reg64,
> >>>>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> >>>>>>> +}
> >>>>>>
> >>>>>> "basis for the second test"? What do you mean? Are you referring
> >>>>>> to the SDM spelling out only one operand order? That's surely a
> >>>>>> (very
> >>>>>> minor) shortcoming there. TEST is commutative, and hence whatever
> >>>>>> order the programmer likes should be accepted by an assembler.
> >>>>>
> >>>>> Yes, I only found "test %ax, (%bx)" and didn't find "test (%bx),
> >>>>> %ax" in
> >>>> the
> >>>>> SDM. And APX spec also doesn’t say " ctest {def=} (%bx), %ax " is
> >>>> supported.
> >>>>> Anyway, their assemblers are same.
> >>>>> I will support "ctestb(%rax),%ecx" and "{evex} test(%rax),%ecx" in
> >>>> assembler,
> >>>>> currently "ctestb(%rax),%ecx" does not work, I need to fix it first.
> >>>>>
> >>>> $ cat test.s
> >>>> test (%edx), %eax
> >>>> test %eax, (%edx)
> >>>>
> >>>> $ as --64  test.s -o test.o
> >>>> $ objdump  -dw -Msuffix test.o
> >>>> 0000000000000000 <_start>:
> >>>>    0:   67 85 02                testl  %eax,(%edx)
> >>>>    3:   67 85 02                testl  %eax,(%edx)
> >>>>
> >>>> Hi HJ,
> >>>>
> >>>> Do you remember why we supported "test (%edx), %eax" in the
> >>>> original version? I'm curious about it.
> >>>
> >>> I don't remember why.  I think we should follow SDM.
> >>
> >> We should follow what makes sense in written code. Other assemblers
> >> permit either order of the operands, when their order doesn't matter.
> >> So should gas do. The fact that the SDM spells out only one order of
> >> operands is (presumably) connected to there being only one encoding
> (unlike for e.g.
> >> CMP). This does not mean we should be unfriendly to people and force
> >> a purely stylistic restriction onto them. And just to mention it: For
> >> TEST both forms were supported virtually forever.
> >> Quite likely simply because, as said, other assemblers to do, too.
> >>
> >
> > Yes, I found that LLVM and NSAM also support "test (%edx), %eax", and llvm
> has just created a jira to follow our decision (They also want to remove it
> instead of continuing to make similar extensions to ctest, but finally it depends
> on us).
> >
> > Personally, since no one knows the original intention of this alias, it seems
> more like a bug to me, because I can't see its benefit, SDM clearly says that
> "When two operands are present in an arithmetic or logical instruction, the
> right operand is the source and the left operand is the destination". "test eax,
> [edx]" is very confusing to me. Someone writing like this would mistakenly
> think that dest is a register, but in fact it is a memory. I think this is a trap, we
> should report an error for it.
> 
> The thing you're apparently missing is that TEST (like CMP) has no destination.
> It has two sources instead.
> 
Aha, got you. 

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 02.07.2024 10:22, Jan Beulich wrote:
> On 02.07.2024 09:09, Cui, Lili wrote:
>>> On 01.07.2024 15:42, Cui, Lili wrote:
>>>> I met some issues when adding CTESTcc and CCMPcc.
>>>>
>>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now looks a bit
>>> weird.
>>>>
>>>> "ccmpbl {dfv=}.s %edx,%eax"
>>>
>>> Well ... Something clearly needs doing about this. I'm now even more convinced
>>> we want tests for these.
>>>
>> Indeed, I added a macro %SW to indicate operands were swapped when suffix_always is true.  Are you ok with it?
> 
> In principle (and if then used consistently), why not. However, as long
> as you use it only for ...
> 
>> --- a/opcodes/i386-dis-evex.h
>> +++ b/opcodes/i386-dis-evex.h
>> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
>>      /* 38 */
>>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
>>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
>> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
>> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
> 
> ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?

I'm sorry, this was half rubbish, I'm afraid. TEST (and hence CTESTcc) has
only a single encoding, and hence would never get .s suffixes. So if there's
no better way, a new %SW macro (just for CCMPcc as you have it above) it is
then. An alternative might be to convert %DF to a DFV_Fixup() handler (to
defer that output enough so that normal .s printing would come first), yet
that may be more overhead than what we have right now for the {dfv=...}
printing.

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> On 02.07.2024 10:22, Jan Beulich wrote:
> > On 02.07.2024 09:09, Cui, Lili wrote:
> >>> On 01.07.2024 15:42, Cui, Lili wrote:
> >>>> I met some issues when adding CTESTcc and CCMPcc.
> >>>>
> >>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now
> >>>> looks a bit
> >>> weird.
> >>>>
> >>>> "ccmpbl {dfv=}.s %edx,%eax"
> >>>
> >>> Well ... Something clearly needs doing about this. I'm now even more
> >>> convinced we want tests for these.
> >>>
> >> Indeed, I added a macro %SW to indicate operands were swapped when
> suffix_always is true.  Are you ok with it?
> >
> > In principle (and if then used consistently), why not. However, as
> > long as you use it only for ...
> >
> >> --- a/opcodes/i386-dis-evex.h
> >> +++ b/opcodes/i386-dis-evex.h
> >> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
> >>      /* 38 */
> >>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
> >>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
> >> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> >> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> >> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
> >> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
> >
> > ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?
> 
> I'm sorry, this was half rubbish, I'm afraid. TEST (and hence CTESTcc) has only
> a single encoding, and hence would never get .s suffixes. So if there's no better
> way, a new %SW macro (just for CCMPcc as you have it above) it is then. An
> alternative might be to convert %DF to a DFV_Fixup() handler (to defer that
> output enough so that normal .s printing would come first), yet that may be
> more overhead than what we have right now for the {dfv=...} printing.
> 

I'm trying to get the opcode from ins->codep and if the opcode > 39 and Msuffix is ​​true, add a ".s" to it.  Do you like this version ?

-    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
-    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
+    { "%NEccmp%SCB%DF",                { Gb, Eb }, 0 },
+    { "%NEccmp%SCS%DF",                { Gv, Ev }, PREFIX_NP_OR_DATA },
     { Bad_Opcode },
     { Bad_Opcode },
     { Bad_Opcode },
diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 978440fa3f1..8cdfd392374 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -10587,6 +10589,11 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
            }
          else if (l == 1 && last[0] == 'D')
            {
+             if (sizeflag & SUFFIX_ALWAYS && (ins->codep[-1] == 0X3A ||ins->codep[-1] == 0X3B))
+               {
+                 *ins->obufp++ = '.';
+                 *ins->obufp++ = 's';
+               }             
              /* Get oszc flags value from register_specifier.  */
              int oszc_value = ~ins->vex.register_specifier & 0xf;

Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 02.07.2024 15:49, Cui, Lili wrote:
>> On 02.07.2024 10:22, Jan Beulich wrote:
>>> On 02.07.2024 09:09, Cui, Lili wrote:
>>>>> On 01.07.2024 15:42, Cui, Lili wrote:
>>>>>> I met some issues when adding CTESTcc and CCMPcc.
>>>>>>
>>>>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now
>>>>>> looks a bit
>>>>> weird.
>>>>>>
>>>>>> "ccmpbl {dfv=}.s %edx,%eax"
>>>>>
>>>>> Well ... Something clearly needs doing about this. I'm now even more
>>>>> convinced we want tests for these.
>>>>>
>>>> Indeed, I added a macro %SW to indicate operands were swapped when
>> suffix_always is true.  Are you ok with it?
>>>
>>> In principle (and if then used consistently), why not. However, as
>>> long as you use it only for ...
>>>
>>>> --- a/opcodes/i386-dis-evex.h
>>>> +++ b/opcodes/i386-dis-evex.h
>>>> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
>>>>      /* 38 */
>>>>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
>>>>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
>>>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
>>>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
>>>> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
>>>> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
>>>
>>> ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?
>>
>> I'm sorry, this was half rubbish, I'm afraid. TEST (and hence CTESTcc) has only
>> a single encoding, and hence would never get .s suffixes. So if there's no better
>> way, a new %SW macro (just for CCMPcc as you have it above) it is then. An
>> alternative might be to convert %DF to a DFV_Fixup() handler (to defer that
>> output enough so that normal .s printing would come first), yet that may be
>> more overhead than what we have right now for the {dfv=...} printing.
>>
> 
> I'm trying to get the opcode from ins->codep and if the opcode > 39 and Msuffix is ​​true, add a ".s" to it.  Do you like this version ?

I'm afraid I don't, since I consider ...

> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> +    { "%NEccmp%SCB%DF",                { Gb, Eb }, 0 },
> +    { "%NEccmp%SCS%DF",                { Gv, Ev }, PREFIX_NP_OR_DATA },
>      { Bad_Opcode },
>      { Bad_Opcode },
>      { Bad_Opcode },
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -10587,6 +10589,11 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>             }
>           else if (l == 1 && last[0] == 'D')
>             {
> +             if (sizeflag & SUFFIX_ALWAYS && (ins->codep[-1] == 0X3A ||ins->codep[-1] == 0X3B))

... this extremely fragile. It was on more than one occasion that I
thought it might be helpful to know the major opcode while handling
operands, but each time I found another solution. First and foremost
to avoid doing what you do here. If we really want to allow access
to the major opcode, it'll need properly storing (along with encoding
space) in "ins".

As a minor remark (may save a comment on a later version), I also
have stylistic concerns with that line (beyond it apparently being
too long). Please can such be written as e.g.

             if ((sizeflag & SUFFIX_ALWAYS) && (ins->codep[-1] | 1) == 0x3B))

?

Jan

RE: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Cui, Lili]

> On 02.07.2024 15:49, Cui, Lili wrote:
> >> On 02.07.2024 10:22, Jan Beulich wrote:
> >>> On 02.07.2024 09:09, Cui, Lili wrote:
> >>>>> On 01.07.2024 15:42, Cui, Lili wrote:
> >>>>>> I met some issues when adding CTESTcc and CCMPcc.
> >>>>>>
> >>>>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now
> >>>>>> looks a bit
> >>>>> weird.
> >>>>>>
> >>>>>> "ccmpbl {dfv=}.s %edx,%eax"
> >>>>>
> >>>>> Well ... Something clearly needs doing about this. I'm now even
> >>>>> more convinced we want tests for these.
> >>>>>
> >>>> Indeed, I added a macro %SW to indicate operands were swapped when
> >> suffix_always is true.  Are you ok with it?
> >>>
> >>> In principle (and if then used consistently), why not. However, as
> >>> long as you use it only for ...
> >>>
> >>>> --- a/opcodes/i386-dis-evex.h
> >>>> +++ b/opcodes/i386-dis-evex.h
> >>>> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
> >>>>      /* 38 */
> >>>>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
> >>>>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
> >>>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> >>>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> >>>> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
> >>>> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
> >>>
> >>> ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?
> >>
> >> I'm sorry, this was half rubbish, I'm afraid. TEST (and hence
> >> CTESTcc) has only a single encoding, and hence would never get .s
> >> suffixes. So if there's no better way, a new %SW macro (just for
> >> CCMPcc as you have it above) it is then. An alternative might be to
> >> convert %DF to a DFV_Fixup() handler (to defer that output enough so
> >> that normal .s printing would come first), yet that may be more overhead
> than what we have right now for the {dfv=...} printing.
> >>
> >
> > I'm trying to get the opcode from ins->codep and if the opcode > 39 and
> Msuffix is ​​true, add a ".s" to it.  Do you like this version ?
> 
> I'm afraid I don't, since I consider ...
> 
> > -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
> > -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
> > +    { "%NEccmp%SCB%DF",                { Gb, Eb }, 0 },
> > +    { "%NEccmp%SCS%DF",                { Gv, Ev }, PREFIX_NP_OR_DATA },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -10587,6 +10589,11 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >             }
> >           else if (l == 1 && last[0] == 'D')
> >             {
> > +             if (sizeflag & SUFFIX_ALWAYS && (ins->codep[-1] == 0X3A
> > + ||ins->codep[-1] == 0X3B))
> 
> ... this extremely fragile. It was on more than one occasion that I thought it
> might be helpful to know the major opcode while handling operands, but each
> time I found another solution. First and foremost to avoid doing what you do
> here. If we really want to allow access to the major opcode, it'll need properly
> storing (along with encoding
> space) in "ins".
> 

Yes, when I want to use major opcode for this case, I think it should be putted under ins, but I didn't find it, then I realized there might be no precedent for disassembler using major opcodes.

> As a minor remark (may save a comment on a later version), I also have
> stylistic concerns with that line (beyond it apparently being too long). Please
> can such be written as e.g.
> 
>              if ((sizeflag & SUFFIX_ALWAYS) && (ins->codep[-1] | 1) == 0x3B))
> 
> ?
Sure, it is better, this usage is common in assembler.

Based on the above discussion, I think it is better to use %sw.

Thanks,
Lili.

Re: [PATCH] x86: Add {load}/{store} tests for apx instructions.

[Jan Beulich]

On 03.07.2024 03:10, Cui, Lili wrote:
>> On 02.07.2024 15:49, Cui, Lili wrote:
>>>> On 02.07.2024 10:22, Jan Beulich wrote:
>>>>> On 02.07.2024 09:09, Cui, Lili wrote:
>>>>>>> On 01.07.2024 15:42, Cui, Lili wrote:
>>>>>>>> I met some issues when adding CTESTcc and CCMPcc.
>>>>>>>>
>>>>>>>> For example, the disassembler for "{load} ccmpbl %edx, %eax" now
>>>>>>>> looks a bit
>>>>>>> weird.
>>>>>>>>
>>>>>>>> "ccmpbl {dfv=}.s %edx,%eax"
>>>>>>>
>>>>>>> Well ... Something clearly needs doing about this. I'm now even
>>>>>>> more convinced we want tests for these.
>>>>>>>
>>>>>> Indeed, I added a macro %SW to indicate operands were swapped when
>>>> suffix_always is true.  Are you ok with it?
>>>>>
>>>>> In principle (and if then used consistently), why not. However, as
>>>>> long as you use it only for ...
>>>>>
>>>>>> --- a/opcodes/i386-dis-evex.h
>>>>>> +++ b/opcodes/i386-dis-evex.h
>>>>>> @@ -940,8 +940,8 @@ static const struct dis386 evex_table[][256] = {
>>>>>>      /* 38 */
>>>>>>      { "%NEccmp%SCB%DF",                { Eb, Gb }, 0 },
>>>>>>      { "%NEccmp%SCS%DF",                { Ev, Gv }, PREFIX_NP_OR_DATA },
>>>>>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
>>>>>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
>>>>>> +    { "%NEccmp%SCB%SW%DF",     { Gb, Eb }, 0 },
>>>>>> +    { "%NEccmp%SCS%SW%DF",     { Gv, Ev }, PREFIX_NP_OR_DATA },
>>>>>
>>>>> ... CCMPcc / CTESTcc, couldn't you make %DF fulfill this job as well?
>>>>
>>>> I'm sorry, this was half rubbish, I'm afraid. TEST (and hence
>>>> CTESTcc) has only a single encoding, and hence would never get .s
>>>> suffixes. So if there's no better way, a new %SW macro (just for
>>>> CCMPcc as you have it above) it is then. An alternative might be to
>>>> convert %DF to a DFV_Fixup() handler (to defer that output enough so
>>>> that normal .s printing would come first), yet that may be more overhead
>> than what we have right now for the {dfv=...} printing.
>>>>
>>>
>>> I'm trying to get the opcode from ins->codep and if the opcode > 39 and
>> Msuffix is ​​true, add a ".s" to it.  Do you like this version ?
>>
>> I'm afraid I don't, since I consider ...
>>
>>> -    { "%NEccmp%SCB%DF",                { Gb, EbS }, 0 },
>>> -    { "%NEccmp%SCS%DF",                { Gv, EvS }, PREFIX_NP_OR_DATA },
>>> +    { "%NEccmp%SCB%DF",                { Gb, Eb }, 0 },
>>> +    { "%NEccmp%SCS%DF",                { Gv, Ev }, PREFIX_NP_OR_DATA },
>>>      { Bad_Opcode },
>>>      { Bad_Opcode },
>>>      { Bad_Opcode },
>>> --- a/opcodes/i386-dis.c
>>> +++ b/opcodes/i386-dis.c
>>> @@ -10587,6 +10589,11 @@ putop (instr_info *ins, const char
>> *in_template, int sizeflag)
>>>             }
>>>           else if (l == 1 && last[0] == 'D')
>>>             {
>>> +             if (sizeflag & SUFFIX_ALWAYS && (ins->codep[-1] == 0X3A
>>> + ||ins->codep[-1] == 0X3B))
>>
>> ... this extremely fragile. It was on more than one occasion that I thought it
>> might be helpful to know the major opcode while handling operands, but each
>> time I found another solution. First and foremost to avoid doing what you do
>> here. If we really want to allow access to the major opcode, it'll need properly
>> storing (along with encoding
>> space) in "ins".
>>
> 
> Yes, when I want to use major opcode for this case, I think it should be putted under ins, but I didn't find it, then I realized there might be no precedent for disassembler using major opcodes.
> 
>> As a minor remark (may save a comment on a later version), I also have
>> stylistic concerns with that line (beyond it apparently being too long). Please
>> can such be written as e.g.
>>
>>              if ((sizeflag & SUFFIX_ALWAYS) && (ins->codep[-1] | 1) == 0x3B))
>>
>> ?
> Sure, it is better, this usage is common in assembler.
> 
> Based on the above discussion, I think it is better to use %sw.

Right, that's what I said earlier. Down the road I may see about doing
this differently, but for now using %SW is going to be okay(ish). My
main dislike about it is how long (and hence increasingly difficult to
follow) "%NEccmp%SCB%SW%DF" is getting.

Jan