[PATCH] D39091: [AArch64][SVE] Asm: Add support for (ADD|SUB)_ZZZ

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: test/MC/AArch64/SVE/assembler_tests/add.s:3
+// RUN: not llvm-mc -triple=aarch64-none-linux-gnu -show-encoding -mattr=-sve 2>&1 < %s | FileCheck --check-prefix=CHECK-ERROR %s
+add     z31.s, z31.s, z31.s  // 00000100-10111111-00000011-11111111
+// CHECK: add     z31.s, z31.s, z31.s // encoding: [0xff,0x03,0xbf,0x04]
----------------
I'm probably showing my ignorance here, but what's with the binary in these comments and CHECK-ERROR-NEXT?


================
Comment at: test/MC/AArch64/SVE/assembler_tests/add.s:5
+// CHECK: add     z31.s, z31.s, z31.s // encoding: [0xff,0x03,0xbf,0x04]
+// CHECK-ERROR: {{(unexpected token in argument list)|(invalid operand for instruction)|(invalid sve vector register)|(unexpected floating point literal)|(expected ']' in brackets expression)|(instruction requires: sve)|(vector register expected)|(immediate must be an integer in range \[-128, 127\] or a multiple of 256 in range \[-32768, 32512\])}}
+// CHECK-ERROR-NEXT: 00000100-10111111-00000011-11111111
----------------
Is this really better than just matching the specific error message? If one of the messages in this list changes in the future, you either have churn across every CHECK-ERROR line in the SVE test cases, or else the CHECK-ERROR lines aren't all updated and you're left with outdated checks. You might also want to know if the error message for a given line changes between one of these unexpectedly.

Of course if FileCheck let you assign a pattern to a variable and reuse it, the trade-off would be different...


================
Comment at: test/MC/AArch64/SVE/disassembler_tests/add.s:5
+# CHECK: add     z31.s, z31.s, z31.s // encoding: [0xff,0x03,0xbf,0x04]
+# CHECK-ERROR-NOT: add     z31.s, z31.s, z31.s 
+0xb7,0x01,0xe8,0x04
----------------
Would it not be better to check that llvm-mc -disassemble -mattr=-sve results in an unknown instruction, rather than "_anything_ other than the specific SVE instruction".

A number of backends have found it useful to combine disassembly with assembly tests in a single file (see e.g. test/MC/RISCV/rv32i-valid.s), and I think you might benefit from a similar "round-trip" testing approach.


https://reviews.llvm.org/D39091