This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Asm: Support for LD1RQ load-and-replicate quad-word vector instructions.
ClosedPublic

Authored by sdesmalen on Apr 30 2018, 1:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Apr 30 2018, 1:59 AM
SjoerdMeijer accepted this revision.Apr 30 2018, 12:47 PM

Looks OK to me.

test/MC/AArch64/SVE/ld1rqb-diagnostics.s
44 ↗(On Diff #144515)

Nit, and feel free to ignore: this "invalid operand" is a really useless error message because it doesn't tell you much (not related/cause by this patch of course). But at least the culprit should be indicated with the carrot symbol, something like this:

ld1rqb z0.h, p0/z, [x0]
       ^

Perhaps nice to match that too because it tells where the error is?

This revision is now accepted and ready to land.Apr 30 2018, 12:47 PM
sdesmalen added inline comments.May 1 2018, 5:12 AM
test/MC/AArch64/SVE/ld1rqb-diagnostics.s
44 ↗(On Diff #144515)

Good suggestion, we can now use the DiagnosticPredicate (https://reviews.llvm.org/D45879) to improve these diagnostics. I'll do that in a separate patch though.

Checking for the caret would be a nice addition, but since FileCheck doesn't check white-space coding this explicitly in the test using a regular expression becomes a bit cumbersome. I noticed this is also not done in any of the other diagnostics files (although I reckon this is not an argument not to do it), but I think the coverage without caret is sufficient if the rest of the instruction is valid.

fhahn added inline comments.May 1 2018, 5:15 AM
test/MC/AArch64/SVE/ld1rqb-diagnostics.s
44 ↗(On Diff #144515)

Checking for the caret would be a nice addition, but since FileCheck doesn't check white-space coding this explicitly in the test using a regular expression becomes a bit cumbersome. I noticed this is also not done in any of the other diagnostics files (although I reckon this is not an argument not to do it), but I think the coverage without caret is sufficient if the rest of the instruction is valid.

Would -strict-whitespace do the trick?

SjoerdMeijer added inline comments.May 1 2018, 5:24 AM
test/MC/AArch64/SVE/ld1rqb-diagnostics.s
44 ↗(On Diff #144515)

True about the whitespaces. There are plenty of tests however that checks that check for "^". For example, the first file I opened: test/MC/AArch64/arm64-diags.s. That would be a bit more meaningful with adding -strict-whitespace indeed, which is used by other files. Anyway, it was just a suggestion, and if you're planning in fixing the diagnostic then that's fine of course.

sdesmalen added inline comments.May 1 2018, 5:35 AM
test/MC/AArch64/SVE/ld1rqb-diagnostics.s
44 ↗(On Diff #144515)

Ah, I didn't know about --strict-whitespace! I agree that would be nice to have, but I'll leave for a later patch as well. Thanks for the suggestions.

This revision was automatically updated to reflect the committed changes.