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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/AArch64/SVEInstrFormats.td:22
+  asm, "\t$Zd, $Zn, $Zm",
+
+  "", []>, Sched<[]> {
----------------
space


================
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
----------------
asb wrote:
> sdesmalen wrote:
> > asb wrote:
> > > 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...
> > One of the things I forgot to mention is that these tests are all auto-generated from our architecture spec. If one of the error messages changes, we can update the generator and create a new set of tests. Even if we had more specific messages, I think we still have to update quite a bunch of tests anyway, but I agree with you that testing for a specific error message makes more sense (and indeed we could pick up on changes between the checked error messages). When we did the assembler/disassembler work, it covered the cases well enough.
> > 
> > That said, I'm happy to make changes to the tests. It is actually a good topic to get feedback on, and we don't necessarily need to stick with the auto-generated approach.
> > 
> > For now though I would rather collect some more input on how the tests should look before making the changes in each patch individually, for the following reasons:
> > 1. If there are some 'batch' changes we want to make, we can easily regenerate the tests.
> > 2. I have currently split up the assembler/disassembler into many, many small patches, including tests. If I would re-generate the tests now, I'd need to fix each patch individually which is a pain :)
> > 
> > When we agree how we want the tests to look and most of the patches are in, I can easily create patches for the tests alone. Does that approach make sense?
> The only problem with auto-generated tests is that if the generator isn't publicly available it's difficult for people who don't have access to extend, refactor or otherwise improve the tests. Auto-generated tests are great, but perhaps you can make it so the test is generated in a form that's closer to how a human would write it?
> 
> Avoiding extra work for yourself while the patches are discussed certainly sounds reasonable to me!
I agree with both arguments from @asb here.

1. We don't want OR in the check lines. We need to make sure we get the error we want, not *any* error, which is what we get now.
2. Auto-generated tests are ok to create the pattern, but once the test is in, it should be human curated (or the generator has to be in LLVM's source repo, separate process).



https://reviews.llvm.org/D39091