This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]SME2 instructions that use ZTO operand
ClosedPublic

Authored by CarolineConcatto on Oct 17 2022, 8:28 AM.

Details

Summary

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

ZERO (ZT0): Zero ZT0.
LDR (ZT0): Load ZT0 register.
STR (ZT0): Store ZT0 register.
MOVT (scalar to ZT0): Move 8 bytes from general-purpose register to ZT0.
     (ZT0 to scalar): Move 8 bytes from ZT0 to general-purpose register.

Consecutive:

LUTI2 (single): Lookup table read with 2-bit indexes.
      (two registers): Lookup table read with 2-bit indexes.
      (four registers): Lookup table read with 2-bit indexes.
LUTI4 (single): Lookup table read with 4-bit indexes.
      (two registers): Lookup table read with 4-bit indexes.
      (four registers): Lookup table read with 4-bit indexes.

The reference can be found here:

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

This patch also adds a new register class and operand for zt0
and a another index operand uimm3s8

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 8:28 AM
CarolineConcatto requested review of this revision.Oct 17 2022, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 8:28 AM
Matt added a subscriber: Matt.Oct 21 2022, 12:39 PM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
557–563

Can I suggest using Zt to represent ZT0 within instruction names (e.g. LUTI2_2ZZtZI. This fits with how we handle other multiple letter operands, for example Pm and Pz for predicate registers.

llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
557–563

We had a discussion about that, the problem is that it may be mistaken with stand-alone Z.
That is the reason we choose to go with T.

Add Instruction naming conventions note

paulwalker-arm added inline comments.Oct 25 2022, 7:38 AM
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
557–563

As mentioned we support Pm, Pz as well as P, which has worked fine for years now. My problem with T is what happens if we start having registers t0-t#.

So I cannot say I buy this answer but fair enough, if this is your preference then so be it.

paulwalker-arm added inline comments.Oct 25 2022, 5:13 PM
llvm/lib/Target/AArch64/AArch64InstrFormats.td
1415

Is this really a memory index? It looks like an offset into the ZT0 vector, so should just be a normal imm/index operand.

1419

I imagine it's not just a simple replacement but as a starting point perhaps use printVectorIndex, given this is how the immediate is used.

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1421

Just an idea but what about calling the register class ZT, which currently contains the single register ZT0?

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

Perhaps add (ZA)?

I think it's worth just committing this comment separately into main given it's already in use by already committed patches.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
3979–3985

It has been a while since I've had to do this but I suspect you really want you're own ParserMethod/tryParse... method if the existing ones don't already work for you.

4190–4193

My nativity showing here but how does zt0 related to vector lists? and why do you need extra handling for za given this patch relates to zt0?

5815

Match_InvalidLookupTable?

5816

To match similar ZA diagnostics how about invalid lookup table, expected zt0?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1247–1253

To me this looks like a sign you're using the wrong print routine.

CarolineConcatto marked 8 inline comments as done.

-Address review comments

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4190–4193

I added because of the test bellow.
But It looks it is not needed.

I've only looked at the parsing side of things so far.

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
1393

This should remain as ZT0 because is represents this specific register.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4348–4349

I think this is only necessary because you're calling tryParseScalarRegister, which looks to be a bit of a hack if I'm honest.

What about updating matchRegisterNameAlias and then calling that instead? Something like:

if ((RegNum = MatchRegisterName(Name)))
  return Kind == RegKind::LookupTable ? RegNum : 0;
4359

I'm guessing you tried tryParseVectorIndex and it didn't work the way you need?

4368

This assumes the next token is a ], which it might not be so you'll assemble something that is not valid syntax. If tryParseVectorIndex doesn't just work, please look how it handles the closing bracket.

5569

Please remove this.

6259

Please move this to be ordered relative to the other Match_InvalidMemoryIndexed entries.

paulwalker-arm added inline comments.Oct 29 2022, 5:13 AM
llvm/lib/Target/AArch64/SMEInstrFormats.td
2385

Can this class represent "SME2 zero lookup table"? then change the one use to pass in opc.

2394

Please pass in opc and opc2 so this class better represents the encoding group.

2420

Please pass this in as a 7 bit opc to match the encoding group.

2432

Please pass this in as a 7 bit opc to match the encoding group.

2437

SME2 Lookup Table Expand One Register

2438

Can you pass in opc and opc2 so this class better represents the encoding group?

2457

With the above suggestion you can use the {1, ?, ?...} syntax when passing the opcode into sme2_luti_vector_index.

The following let Inst{17-14} = i is fine as is.

See sve_int_bin_pred_shift_imm_left for what I mean. Your code placement is fine (i.e. there's no need to move anything into multiclass sme2_luti2_vector_index to match the shift example).

2480

Please add a comment containing the name of the encoding group.

Much like my comment on sme2_luti2_vector_index can you pass in opc and opc2 so this class better represents the encoding group.

2525

Same comments as with sme2_luti_vector_vg2_index.

CarolineConcatto marked 14 inline comments as done.
  • Address review comments
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4348–4349

Oh..I can use matchRegisterNameAlias.
By looking others tryParse<smth> it looked like I should use tryParseScalarRegister.

4348–4349

I created a RegKind::LookupTable. But don't know if that is what you meant.
Is that correct?

4359

CreateVectorIndex I cannot parse
But I can with CreateImm

4368

It has now almost the same code as tryParseVectorIndex.
But for tryParseZTOperand it creates an Imm

Operands.push_back(AArch64Operand::CreateImm(

and for tryParseVectorIndex it creates a vector index:

Operands.push_back(AArch64Operand::CreateVectorIndex(
paulwalker-arm accepted this revision.Nov 1 2022, 9:46 AM

A few minor suggestions but otherwise this looks good to me.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
2796–2797

Personally I think the following reads better.

return (Kind == RegKind::Scalar) || (Kind == RegKind::LookupTable) ? RegNum : 0;
4348–4349

Yes, the RegKind::LookupTable you've created is what I had in mind. Thanks.

4349–4350

Is this required? now that matchRegisterNameAlias is being used I expect its (RegNum == 0) check to be enough?

4368

I think we should be able to use the existing VectorIndex code with a bit of extra tweaking but for now I good with this.

4370

stray ;

4374

stray ;

5587

Please move this so it sits with the related Match_InvalidMemoryIndexed diagnostics (i.e. just before Match_InvalidMemoryIndexed8UImm5?)

6263

As above, there's an existing group for Match_InvalidMemoryIndexed#UImm# diagnostics.

This revision is now accepted and ready to land.Nov 1 2022, 9:46 AM
CarolineConcatto marked 5 inline comments as done.
  • Address review comments
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4349–4350

I still need the test here, because it could be also a scalar registers.
Because it uses parseRegister, all registers will go through tryParseZTOperand, including the scalars, that use matchRegisterNameAlias. The only difference between the scalar and ZT is the name.

paulwalker-arm added inline comments.Nov 2 2022, 11:13 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4349–4350

OK, I see what you mean. In which case I believe this code it just masking a bug in matchRegisterNameAlias and shows you cannot rely on MatchRegisterName for zt0. Instead we likely want something like

if (Name.equals_insensitive("zt0"))
    return Kind == RegKind::LookupTable ? AArch64::ZT0 : 0;

just after the matchMatrixRegName block. I guess you could create matchLookupTableRegName but given there's only one it doesn't seem worth it.

This revision was automatically updated to reflect the committed changes.
CarolineConcatto marked an inline comment as done.Nov 3 2022, 12:48 AM
c-rhodes added inline comments.
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
74

this new RegKind is causing a warning

/home/culrho01/llvm-project/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2823:11: warning: enumeration value 'LookupTable' not handled in switch [-Wswitch]
  switch (K) {
          ^
4346–4347

this is causing a warning:

/home/culrho01/llvm-project/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:4347:20: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
  StringRef Name = Tok.getString().lower();
                   ^~~~~~~~~~~~~~~~~~~~~~~

you have to be careful with StringRef::lower it returns an std::string and storing back to StringRef can cause weird behavior. In this case you don't need to lower at all anyway since matchRegisterNameAlias does a case insensitive match.

c-rhodes added inline comments.Nov 3 2022, 3:32 AM
llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4355

this is the same as StartLoc since no tokens have been lexed between calls, Lex should come before this

4365

this error message needs updating