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
574–580

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
574–580

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
574–580

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
1380

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

1384

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
1300

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
3871–3877

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.

4089–4092

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?

5653

Match_InvalidLookupTable?

5654

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

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp
1218–1226

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
4089–4092

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
1272

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

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
4245–4246

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;
4256

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

4265

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.

5428

Please remove this.

6096

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
2399

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

2408

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

2434

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

2446

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

2451

SME2 Lookup Table Expand One Register

2452

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

2471

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).

2494

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.

2539

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
4245–4246

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

4245–4246

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

4256

CreateVectorIndex I cannot parse
But I can with CreateImm

4265

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
2716–2717

Personally I think the following reads better.

return (Kind == RegKind::Scalar) || (Kind == RegKind::LookupTable) ? RegNum : 0;
4245–4246

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

4246–4247

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

4265

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.

4267

stray ;

4271

stray ;

5440

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

6099

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
4246–4247

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
4246–4247

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
73

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) {
          ^
4243–4244

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
4252

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

4262

this error message needs updating