This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE2] Add the SVE2.1 contiguous load to multiple consecutive vector
ClosedPublic

Authored by david-arm on Oct 25 2022, 6:30 AM.

Details

Summary

This patch adds the assembly/disassembly for the following instructions:

ld1* : Contiguous load of bytes to multiple consecutive vectors -
(scalar + scalar) and (scalar + immediate)
ldnt1* : Contiguous load non-temporal of bytes to multiple consecutive
vectors - (scalar + scalar) and (scalar + immediate)

The reference can be found here:
https://developer.arm.com/documentation/ddi0602/2022-09

Diff Detail

Event Timeline

david-arm created this revision.Oct 25 2022, 6:30 AM
david-arm requested review of this revision.Oct 25 2022, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 6:30 AM
Matt added a subscriber: Matt.Oct 29 2022, 8:21 AM
paulwalker-arm added inline comments.Oct 30 2022, 7:33 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
3600

It looks like we were not as consistent with adding register suffixes for the SVE loads and store as with the other instructions. I see two options:

  1. Match the existing naming but include register counts (e.g. something like LD1B_2Z and LD1B_2Z_IMM, or
  2. Reference all operand registers as you've done but include the zeroing aspect like we usually do for predicated instructions (i.e. LD1B_2ZCzXX and LD1B_2ZCzXI).

Personally I prefer option 1 but the choice is yours.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3863–3878

Is there now sufficient commonality that you can reuse tryParseSVEPredicateVector and just pass in the register kind or have that as a template parameter? I know this only requires "\z" support, but I don't think that really matters does it?

david-arm updated this revision to Diff 472000.Oct 31 2022, 7:33 AM
  • Renamed LD1 instructions.
  • Removed tryParseSVEPredicateAsCounter and attempted to reuse the existing tryParseSVEPredicateVector instead.
david-arm marked 2 inline comments as done.Oct 31 2022, 7:36 AM
david-arm added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3863–3878

It's not as straightforward to reuse as it looks, because:

  1. We want an error message for "pn/m", whereas "/m" is valid for normal predicates, and
  2. The parsing of predicate indices is slightly different for each, i.e. "[w12, 0]" is a valid index for predicates (e.g. psel p0, p0, p0.b[w12, 0]), but not for predicate-as-counters.

I've tried my best to reuse the code though, with some minor template-specialisations.

paulwalker-arm accepted this revision.Oct 31 2022, 10:54 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3911–3917

To be honest I don't see much value in this change. It's not really true that SVEPredicateVector generally expects 'm' or 'z' because in reality few instructions support z and even less support both z and m. For example:

add     z23.b, p3/j, z23.b, z13.b
add     z23.b, p3/z, z23.b, z13.b
add     z23.b, p3/m, z23.b, z13.b

gives:

	.text
<stdin>:1:19: error: expecting 'm' or 'z' predication
add     z23.b, p3/j, z23.b, z13.b
                  ^
<stdin>:2:19: error: invalid operand for instruction
add     z23.b, p3/z, z23.b, z13.b
                  ^
	add	z23.b, p3/m, z23.b, z13.b       // encoding: [0xb7,0x0d,0x00,0x04]

This is the precedent set for predicate diagnostics and whilst not perfect I don't see a massive reason for SVEPredicateAsCounter to diverge. Especially given as soon as m support is required, removing this change will likely be the chosen fix.

This revision is now accepted and ready to land.Oct 31 2022, 10:54 AM
This revision was landed with ongoing or failed builds.Nov 1 2022, 2:32 AM
This revision was automatically updated to reflect the committed changes.
david-arm marked an inline comment as done.