This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] SME2 Single-multi vector ternary int/FP 2 and 4 registers
ClosedPublic

Authored by CarolineConcatto on Oct 7 2022, 9:15 AM.

Details

Summary

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

For INT:

ADD(array results, multiple and single vector): Add replicated single
    vector to multi-vector with ZA array vector results.
SUB(array results, multiple and single vector): Subtract replicated single
    vector from multi-vector with ZA array vector results.

For FP:

FMLA (multiple and single vector): Multi-vector floating-point fused
      multiply-add by vector.
FMLS (multiple and single vector): Multi-vector floating-point
      multiply-subtract long by vector.

The reference can be found here:

https://developer.arm.com/documentation/ddi0602/2022-09

The Matriz Operand has 2 new sizes 32(.s) and 64(.d) bits
(MatrixOp32 and MatrixOp64)

Depends on: D135448

Depends on: D135952

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 9:15 AM
CarolineConcatto requested review of this revision.Oct 7 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 9:15 AM
Matt added a subscriber: Matt.Oct 10 2022, 10:51 PM
sdesmalen added inline comments.Oct 12 2022, 7:34 AM
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1043

nit: Can you add /*Stride=*/2, /*PrintRange=*/true to make it clear what these operands mean?

1404

Can you move these definitions to line 1368 (after def MatrixOp : ... ?)

llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
250

nit: please remove newline

262

nit: please remove newline

267

nit: please remove newline

276

Did you mean mla_add_sub ?

281

nit: please remove newline

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

drop const?

4241

nit: s/!VG.size()/VG.empty()/

llvm/lib/Target/AArch64/SMEInstrFormats.td
1195

nit: s/ /, /

-address comments

CarolineConcatto marked 9 inline comments as done.Oct 12 2022, 9:33 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3145

It complains:
: cannot bind non-const lvalue reference of type ‘llvm::Optional<std::pair<int, int> >&’ to an rvalue of type ‘llvm::Optional<std::pair<int, int> >’

parseVectorKind(Name.drop_front(DotPosition), RegKind::Matrix);
sdesmalen added inline comments.Oct 13 2022, 6:30 AM
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1043

For a vector-list of consecutive registers we should align NEON, SVE and SME2 here, such that:

{z0.s, z1.s} vs {z0.s - z1.s} -> prefer {z0.s, z1.s}
{z0.s, z1.s, z2.s} vs {z0.s - z2.s} -> prefer {z0.s - z2.s}
{z0.s, z1.s, z2.s, z3.s} vs {z0.s - z3.s} -> prefer {z0.s - z3.s}

This means that we don't need a special ZZ_s_r, and we should change ZZZ_* and ZZZZ_* to print the range form (rather than adding new ZZZZ_*_r` forms as you did in this patch).

Changing this for SVE(2) also means updating the corresponding tests. Can you pull those changes out into a separate patch and then rebase this patch on top?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1429 ↗(On Diff #467174)

s/Stride/N/

1493 ↗(On Diff #467174)

Stride is a misnomer, because the stride is actually '1' (the registers are consecutive). This should be 'N', the number of registers in the range.

1506 ↗(On Diff #467174)

This should be N, not stride.

CarolineConcatto marked 4 inline comments as done.
  • Patch depend on D135952
  • Fix GlobalIsel test error.
  • Change mir codegen test, but needs someone else to have a look if changes are ok.

Mainly:

llvm/test/CodeGen/AArch64/stp-opt-with-renaming-reserved-regs.mir
llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1043

This patch covers your suggestion:
https://reviews.llvm.org/D135952

The GISel test changes are fine but maybe someone should look into the STP test changes.

llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
367–371

The STP optimization isn't firing any more, this might be a problem with either the test or the optimization itself. IIRC @c-rhodes has looked into this before.

CarolineConcatto edited the summary of this revision. (Show Details)
  • Change commit message to have 2 depends on. Maybe this will fix problems with

the failing tests

  • Rebase on top of changes in D135952
sdesmalen added inline comments.Oct 18 2022, 6:05 AM
llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1372

Should this register class have let DiagnosticType = "InvalidMatrixIndexGPR32_8_11" with corresponding support in the switch-statements in AArch64AsmParser.cpp?

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

Should this be part of this patch? Or was this supposed to be part of D135448 ?

4320–4321

nit: odd line-break (not at 80char limit)

llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
367–371

The issue seems to be caused by adding new register classes which seems to 'confuse' the AArch64LoadStoreOptimizer. @c-rhodes created D88663 a while back to fix this, but I haven't tried to see if that patch actually fixes the issue in this test. In any case the issue is exposed by this patch, not really caused by it (which can be seen by just adding the register class in isolation), so I think the issue should be fixed independently from this patch.

540

Why has the test itself changed here?

CarolineConcatto marked an inline comment as done.
  • address review comments
CarolineConcatto marked 2 inline comments as done.Oct 19 2022, 1:34 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3314

Yes, maybe. But this is first patch that I added sme2 instructions, that is the reason I only added now. What would you like me to do? Remove from this line from this patch and create another one, only to add this flag or is it fine if we leave here?

sdesmalen added inline comments.Oct 19 2022, 5:38 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3314

I mostly just wanted to understand it. Sounds like it couldn't be tested before, but in your current patch you're not testing this change either. Maybe you can add a test similar to llvm/test/MC/AArch64/SVE2/directive-arch.s, but then for SME2? (we should have added a similar test for SME).

CarolineConcatto marked an inline comment as done.
  • Add directive test to cover feature flag added on AArch64AsmParser.cpp
  • Rebase on top of master
sdesmalen accepted this revision.Oct 19 2022, 8:33 AM
This revision is now accepted and ready to land.Oct 19 2022, 8:33 AM
This revision was landed with ongoing or failed builds.Oct 19 2022, 9:51 AM
This revision was automatically updated to reflect the committed changes.