This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Asm: More concise test format
ClosedPublic

Authored by sdesmalen on Nov 8 2017, 4:24 AM.

Details

Summary

Change the test format for SVE assembler/disassembler tests to be less verbose and have both tests in the same file.

The tests check the following:

  • All instructions are assembled correctly into the right encoding.
  • All instructions are disassembled correctly (into the preferred assembly format)
  • Without -mattr=+sve the instructions are not assembled.
  • Without -mattr=+sve the instructions are not disassembled.

This patch also adds several negative tests for SVE add/sub.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 8 2017, 4:24 AM
fhahn edited edge metadata.Nov 8 2017, 4:36 AM

Thanks Sander, it looks like you incorporated the suggestions from D39091 and I think with the new format it is much easier to see what's going on!

That being said, it seems like we do not test passing invalid operands to SVE instructions (with +sve), , i.e. all assembler instructions are valid SVE instructions. We should also have tests with invalid operands, e.g. sub z99.s, z0.s, z0.s, sub z0.X, z0.X, z0.X, sub z0.s, z0.h, z0.d, sub x0, z0.h, z0.h.

rengolin edited edge metadata.Nov 8 2017, 4:42 AM

Much better indeed! I agree with Florian on the additional "bad arguments" tests. Thanks!

asb edited edge metadata.Nov 8 2017, 4:44 AM

This looks good to me. Tests for invalid operands etc as Florian suggests are definitely worth having. ARM, Mips and RISCV tend to add this in test files prefixed or suffixed with invalid, e.g. add-invalid.s or sve-invalid.s. AArch64 seems to use a diagnostics suffix.

Thanks for the quick feedback all! Indeed, negative testing is important. Most of the instructions that I'm planning to add we have a separate set of negative tests. This includes ADD|SUB, although the current set of negative tests currently addresses other cases like e.g. out-of-bound immediates, not yet exercised by the unpredicated add/sub from my previous patch. I'll see if I can update the negative-test generator to include the cases mentioned by Florian.

sdesmalen updated this revision to Diff 122408.Nov 10 2017, 2:48 AM
sdesmalen edited the summary of this revision. (Show Details)

Added negative tests.

One of the negative tests actually exposed a bug where the same diagnostic is produced several times (it does this because it is unmatched for each register operand class, rather than bail out with a parse failure when z31.x is given). I have a fix for that, but will address it in a separate patch so not to complicate this patch.

fhahn accepted this revision.Nov 10 2017, 3:02 AM

LGTM, thanks. It might be worth to add a case where GPRs and vector registers are passed. Please wait a bit with committing, to give people a chance to raise additional comments.

This revision is now accepted and ready to land.Nov 10 2017, 3:02 AM
rengolin accepted this revision.Nov 10 2017, 3:16 AM

One of the negative tests actually exposed a bug where the same diagnostic is produced several times (it does this because it is unmatched for each register operand class, rather than bail out with a parse failure when z31.x is given). I have a fix for that, but will address it in a separate patch so not to complicate this patch.

Excellent! :)

I agree the fix is better at a different patch. LGTM, thanks!

sdesmalen updated this revision to Diff 122438.Nov 10 2017, 8:03 AM

I had accidentally changed one of the diagnostic messages in the negative tests, causing the two tests to fail. This is now fixed and the tests all pass.

This revision was automatically updated to reflect the committed changes.
test/MC/AArch64/SVE/disassembler_tests/add.s