This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Add matrix register definitions and parsing support
ClosedPublic

Authored by c-rhodes on Jul 7 2021, 11:29 AM.

Details

Summary

SME introduces the ZA array, a new piece of architectural register state
consisting of a matrix of [SVLb x SVLb] bytes, where SVL is the
implementation defined Streaming SVE vector length and SVLb is the
number of 8-bit elements in a vector of SVL bits.

SME instructions consist of three types of matrix operands:

  • Tiles: a ZA tile is a square, two-dimensional sub-array of elements within the ZA array. These tiles make up the larger accumulator array and the granularity varies based on the element size, i.e.
    • ZAQ0..ZAQ15 (smallest tile granule)
    • ZAD0..ZAD7
    • ZAS0..ZAS3
    • ZAH0..ZAH1 or ZAB0 (largest tile granule, single tile)
  • Tile vectors: similar to regular tiles, but have an extra 'h' or 'v' to tell how the vector at [reg+offset] is layed out in the tile, horizontally or vertically. E.g. za1h.h or za15v.q, which corresponds to vectors in registers ZAH1 and ZAQ15, respectively.
  • Accumulator matrix: this is the entire accumulator array ZA.

This patch adds the register classes and related operands and parsing
for SME instructions operating on the accumulator array.

The ADDHA and ADDVA instructions which operate on tiles are also added
in this patch to make some use of the code added, later patches will
make use of the other operands introduced here.

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

Co-authored by: Sander de Smalen (@sdesmalen)

Diff Detail

Event Timeline

c-rhodes created this revision.Jul 7 2021, 11:29 AM
c-rhodes requested review of this revision.Jul 7 2021, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 11:29 AM
Matt added a subscriber: Matt.Jul 7 2021, 2:59 PM
Matt removed a subscriber: Matt.Jul 7 2021, 3:01 PM
Matt added a subscriber: Matt.
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1182

Should it be all aligned?

1321

nit: remove extra space

1324

nit: Should you align all elements?
The last ones are not.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2474

Should za be here too?
let SubRegIndices = [zasubb] in {

def ZA : AArch64Reg<0, "za", [ZAB0]>;

}

c-rhodes updated this revision to Diff 357877.Jul 12 2021, 3:23 AM
  • Address @CarolineConcatto's comments.
  • Updated the sed line from sed '/\.text$/d' | sed 's/.*encoding:\s//g' -> sed '/.text/d' | sed 's/.*encoding: //g' in positive tests. Hopefully fixes the failing Windows precommit.
c-rhodes marked 3 inline comments as done.Jul 12 2021, 3:48 AM
c-rhodes added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2474

Should za be here too?

I think so but adding it breaks the smstart za instruction that aliases MSR SVCRZA, #1, introduced in D105576. Although having looked into it I was a bit surprised to find za gets parsed as a scalar register and by adding it to the switch it matches as a matrix in matchRegisterNameAlias, but since the kind it's matching against (scalar) doesn't match it no longer parses. It's a bit of fluke the alias gets parsed so I'll add za to the switch here and see if I can fix the parsing for the alias in D105576.

Hi @c-rhodes, I've still got a few files to review, but I've left a few comments so far!

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
385

I think this should be unsigned as well to match the definition of getMatrixElementWidth

1510

unsigned to match getMatrixElementWidth?

2109

unsigned here too I think

3003

Can this assert be hit easily in practice by simply typing the wrong assembly, i.e.

"addha za0hb"

?

3009

It looks like if I write assembly like this:

addha za0p.d

then we'll treat this as a matrix tile. Do we have any tests that show us treating this type of register as an error?

llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp
648

Should this be 5 instead of 4 to match the number of entries?

c-rhodes updated this revision to Diff 358206.Jul 13 2021, 2:36 AM

Address @david-arm's comments.

c-rhodes marked 2 inline comments as done.Jul 13 2021, 2:41 AM

Hi @c-rhodes, I've still got a few files to review, but I've left a few comments so far!

thanks for reviewing Dave

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
385

ah yeah, good spot

3003

Can this assert be hit easily in practice by simply typing the wrong assembly, i.e.
"addha za0hb"

No, since that assembly won't match in matchMatrixRegName so it should return no match above.

3009

It looks like if I write assembly like this:

addha za0p.d

then we'll treat this as a matrix tile. Do we have any tests that show us treating this type of register as an error?

Same as above, that's not a valid reg name so it shouldn't match.

llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp
648

Should this be 5 instead of 4 to match the number of entries?

Yeah good spot, both values were wrong, 15 should be 16.

david-arm added inline comments.Jul 13 2021, 3:30 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3009

Is it possible to have at least one negative test case for this to show we print an error?

c-rhodes updated this revision to Diff 358237.Jul 13 2021, 5:09 AM

Add negative test for invalid matrix register

c-rhodes marked an inline comment as done.Jul 13 2021, 5:10 AM
c-rhodes added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3009

Is it possible to have at least one negative test case for this to show we print an error?

I've added a few tests to addha-diagnostics.s

david-arm accepted this revision.Jul 13 2021, 6:17 AM

LGTM! Perhaps fix a couple of formatting issues before merging?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
891

nit: Perhaps it's worth fixing the formatting here? I looked in this file and there seems to be a mixture of styles so it's not obvious we should stick to an older style.

llvm/unittests/Target/AArch64/MatrixRegisterAliasing.cpp
23

nit: Maybe fix formatting here?

This revision is now accepted and ready to land.Jul 13 2021, 6:17 AM
c-rhodes marked an inline comment as done.Jul 13 2021, 6:50 AM

LGTM! Perhaps fix a couple of formatting issues before merging?

Sure, I'll run clang-format before landing. Thanks

This revision was landed with ongoing or failed builds.Jul 14 2021, 1:26 AM
This revision was automatically updated to reflect the committed changes.