This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]SME2 Single and multiple vectors SVE Destructive two/four registers[part2]
ClosedPublic

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

Details

Summary

This patch adds the assembly/disassembly for the following instruction:
INT:

SMAX (multiple and single vector): Multi-vector signed maximum by vector.
     (multiple vectors): Multi-vector signed maximum.
SMIN (multiple and single vector): Multi-vector signed minimum by vector.
     (multiple vectors): Multi-vector signed minimum.
UMAX (multiple and single vector): Multi-vector unsigned maximum by vector.
     (multiple vectors): Multi-vector unsigned maximum.
UMIN (multiple and single vector): Multi-vector unsigned minimum by vector.
     (multiple vectors): Multi-vector unsigned minimum.
SRSHL (multiple and single vector): Multi-vector signed rounding shift left by vector.
      (multiple vectors): Multi-vector signed rounding shift left.
URSHL (multiple and single vector): Multi-vector unsigned rounding shift left by vector.
      (multiple vectors): Multi-vector unsigned rounding shift left.

FP:

FMAX (multiple and single vector): Multi-vector floating-point maximum by vector.
     (multiple vectors): Multi-vector floating-point maximum.
FMAXNM (multiple and single vector): Multi-vector floating-point maximum number by vector.
       (multiple vectors): Multi-vector floating-point maximum number.
FMIN (multiple and single vector): Multi-vector floating-point minimum by vector.
     (multiple vectors): Multi-vector floating-point minimum.
FMINNM (multiple and single vector): Multi-vector floating-point minimum number by vector.
       (multiple vectors): Multi-vector floating-point minimum number.

The reference can be found here:

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

It also updates ADD and SQDMULH

Depends on: D135563

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 9:15 AM
CarolineConcatto requested review of this revision.Oct 10 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 9:15 AM
Matt added a subscriber: Matt.Oct 10 2022, 10:53 PM

The output of the tests "CHECK-INST:" need to be updated, but it is still possible to review the patch

CarolineConcatto edited the summary of this revision. (Show Details)Oct 21 2022, 8:43 AM
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
373–383

Just related to layout but it's more typical to group instructions based on the multiclass rather instruction name.

llvm/lib/Target/AArch64/SMEInstrFormats.td
1867–1868

There doesn't look to be much commonality between the derived classes and the splitting of the Inst fields makes it hard to spot bugs. I guess I'm asking why the base class is advantageous over implementing sme2_max_min_vector_vg2_single, sme2_max_min_vector_vg4_single,... as isolated instruction classes.

1889

Is the line wrapping here necessary? Typically we only follow the 80 character limit for these td files when it comes to comments and class declarations.

-Fix tests that were not updated correct

  • address review comments
CarolineConcatto marked an inline comment as done.Oct 21 2022, 10:48 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
373–383

Do you want me to group them in this patch by group of instructions?
Sorry, but I don't understand what you would like me to fix.
I can group them here, and do another NFC for the other instructions, otherwise things will get a bit moew random.

llvm/lib/Target/AArch64/SMEInstrFormats.td
1867–1868

I created a class for each group now.
I hope that is better now to review.

paulwalker-arm added inline comments.Oct 22 2022, 4:56 AM
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
373–383

If you look at AArch64SVEInstrInfo.td you'll see that typically we group things based on class names, so in this case I'd expect

defm SMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"smax", 0b00>;
defm UMAX_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"umax", 0b01>;
defm SMIN_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"smin", 0b10>;
defm UMIN_VG2_2ZZ  : sme2_int_max_min_vector_vg2_single<"umin", 0b11>;

defm SMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"smax", 0b00>;
defm UMAX_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"umax", 0b01>;
defm SMIN_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"smin", 0b10>;
defm UMIN_VG4_4ZZ  : sme2_int_max_min_vector_vg4_single<"umin", 0b11>;

However this is purely stylistic and I can see AArch64SMEInstrInfo.td uses a mixture of styles so this can be cleaned up in a following patch or ignored given I suppose there's no rule to say both should follow the same format.

llvm/lib/Target/AArch64/SMEInstrFormats.td
1867–1868

Thanks Carol, this helps a lot. The similarities are clearer which prompted me to look at the encoding groups and I think you've perhaps gone one level too far down? I can see the encoding group "SME2 Multi-vector - Multiple and Single SVE Destructive (Two registers)" which these instructions below, amongst others which all use the same format and so perhaps you can have one instruction class which all the similar instructions (min, max, fmin, fmax, shift, add...) can use. Sure the opcode space is sparse but that's fine. It'll just mean it's easier to add a new instruction within this group later on.

The same goes for the "Four registers" variant where I see "SME2 Multi-vector - Multiple and Single SVE Destructive (Four registers)", so again one instruction class can likely be used for all the instructions it contains.

paulwalker-arm added inline comments.Oct 22 2022, 5:03 AM
llvm/lib/Target/AArch64/SMEInstrFormats.td
1867–1868

Just to avoid confusion here. I'm not saying we don't need the multiclasses like sme2_fp_max_min_vector_vg2_single and sme2_int_max_min_vector_vg2_single. We need those to account for the differences in supported element sizes. However, I think they can inherit from the same "SME2 Multi-vector - Multiple and Single SVE Destructive (Two registers)" instruction class.

CarolineConcatto marked an inline comment as done.
CarolineConcatto retitled this revision from [AArch64]SME2 Single and multiple vectors Int/FP min/max two/four registers to [AArch64]SME2 Single and multiple vectors SVE Destructive two/four registers[part2].
CarolineConcatto edited the summary of this revision. (Show Details)
  • group all SVE destructive instructions

Remove unwanted changes

CarolineConcatto marked an inline comment as done.Oct 24 2022, 12:12 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/SMEInstrFormats.td
1867–1868

Hey Paul,

So I believe all the SVE destructive instructions are using common classes.
Is that what you meant?

Thanks @CarolineConcatto, this is a much nicer structure and in keeping with how we typically add new instructions.

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

Should this be SMAX_VG4_4Z4Z? If yes then you'll need to check all new defs in this patch because I've seen this potentially incorrect naming several times.

Whilst mentioning consistency the choice of whether to use _VG# seems arbitrary or perhaps I'm misinterpreting it's meaning. For example SRSHL_2Z2Z looks like it should have _VG2 but doesn't. That said, given we specify the register types within the instructions (i.e. _2Z2Z) I'm wondering if most all the _VG# usages are redundant?

Redundancy is likely a question for later but I do think you should be naming the instructions consistently.

llvm/lib/Target/AArch64/SMEInstrFormats.td
1439–1440

Is there value in breaking this up? instead of just having Inst{10-5} = op{6-1}.

1473–1474

As above, do these need to be split?

1507–1509

Can you make f part of the opcode? much like you did for the _single variants.

1540–1542

As above, can these be packed together?

CarolineConcatto marked 5 inline comments as done.

-Address comments review

  • Fix inconsistency In the instructions name
  • Remove f from the class
CarolineConcatto marked an inline comment as done.Oct 24 2022, 1:40 PM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
376

I agree that the naming needs to be consistent. I missed that.
Thank you for noticing. I will double-check the following patches for name consistency.

paulwalker-arm accepted this revision.Oct 24 2022, 4:19 PM
This revision is now accepted and ready to land.Oct 24 2022, 4:19 PM
This revision was landed with ongoing or failed builds.Oct 25 2022, 1:05 AM
This revision was automatically updated to reflect the committed changes.
CarolineConcatto marked an inline comment as done.