This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]SME2 Outer Product and Accumulate instructions
ClosedPublic

Authored by CarolineConcatto on Oct 17 2022, 6:31 AM.

Details

Summary

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

BMOPA: Bitwise exclusive NOR population count outer product and accumulate.
BMOPS: Bitwise exclusive NOR population count outer product and subtract.

SMOPA (2-way): Signed integer sum of outer products and accumulate.
SMOPS (2-way): Signed integer sum of outer products and subtract.

UMOPA (2-way): Unsigned integer sum of outer products and accumulate.
UMOPS (2-way): Signed integer sum of outer products and accumulate.

The reference can be found here:

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 6:31 AM
CarolineConcatto requested review of this revision.Oct 17 2022, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2022, 6:31 AM
Matt added a subscriber: Matt.Oct 21 2022, 12:39 PM
  • Re-use sme class
  • Rebase
  • Rebase correct(hopefully)

I'm only really bothered about the instruction names. My other suggestions are nice to have but only if you agree they'll make things better.

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

Should this be BMOPA_MPPZZ_S because it uses ZA not ZT0?

567

The equivalent SME1 instruction is SMOPA_MPPZZ_S so I'd expect this to be named similar but much like with the DOT instructions you'll need to rename the original to reflect the difference between 4way and 2way. So the original becomes SMOPA_MPPZZ_BtoS and SMOPA_MPPZZ_HtoD and this new one becomes SMOPA_MPPZZ_HtoS. Do you agree?

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

Up to you but given the change perhaps bit u0, bit u1, bit S is better expressed as a 3bit opcode? It seems weird seeing opc{2}, opc{1}, opc{0}. I'm almost tempted to suggest all the operands are just a 5 bit opcode but I'll leave it up to you if you want to make such a change.

148

Again it's up to you but this just looks like a 3bit opcode to me.

CarolineConcatto marked 3 inline comments as done.
  • Address review commentsy
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
564

Now, I am not sure if we should do the same for BMOPA.
In sme1 BFMOPA do not have a size, but FMOPA_MPPZZ has(but because it has a D and S size)

567

I had changed for SME1, but these instructions name are in use in AArch64ISelLowering.
So I did not changed for sme1, but did for sme2.
TBH I don't know what is more consistent, changing or not changing the names for sme2.

paulwalker-arm accepted this revision.Oct 28 2022, 8:37 AM

I still want BMOPA_MPPZZ/BMOPS_MPPZZ to include an element suffix but otherwise looks good.

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

Most all instructions have a size. It is just that some only support a single element type. In such cases we should still add the size suffix so _S in this case because otherwise if a new variant of bmopa is added later we'll be forced to have to rename stuff, much like with the smopa discussion.

567

We should change the old names to be consistent. It doesn't matter that they are used by AArch64ISelLowering because it's just a textual change. It's not that the old names were wrong when originally added, but based on these new SME2 instructions the old names will lead to confusion, hence the new _HtoS style for such things.

With all that said, this assembler work is more important so feel free to defer that refactoring until the assembler support is complete.

This revision is now accepted and ready to land.Oct 28 2022, 8:37 AM
CarolineConcatto marked an inline comment as done.Oct 28 2022, 9:55 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td
567

Thank you Paul, I will create a to do for that.

This revision was landed with ongoing or failed builds.Oct 28 2022, 9:56 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Oct 28 2022, 11:52 AM

Hi @CarolineConcatto, your change seems to be causing a test failure, can you take a look and revert if it will take a while to resolve?
https://lab.llvm.org/buildbot/#/builders/188/builds/21487

Note, the same test also failed in the pre-commit check builds: https://reviews.llvm.org/B194938

nathanchance added a subscriber: nathanchance.EditedOct 28 2022, 3:31 PM

I just bisected check-llvm failing to this change. Based on that and the above comment and the fact that it does not look like there is any progress on a forward fix that I can see (presumably due to time zones), I reverted this change in 23c50432fb25775fe0eb958bc8c2a099b0a0c286.