Page MenuHomePhabricator

[AArch32] Armv8.6a Matrix Mul Assembly Parser Support
ClosedPublic

Authored by LukeGeeson on Apr 10 2020, 6:44 AM.

Details

Summary

This patch upstreams support for the Armv8.6-a Matrix Multiplication
Extension. A summary of the features can be found here:

https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/arm-architecture-developments-armv8-6-a

This patch includes:

  • Assembly support for AArch32 and Assembly Parsing

D77872 has already added the MC representations of the instructions so that they can be used in code gen; this patch fills in the details needed to make assembly parsing work, and adds tests for asm and disasm

This is part of a patch series, starting with BFloat16 support and
the other components in the armv8.6a extension (in previous patches
linked in phabricator)

Based on work by:

  • Luke Geeson
  • Oliver Stannard
  • Luke Cheeseman

Diff Detail

Event Timeline

LukeGeeson created this revision.Apr 10 2020, 6:44 AM

Harbourmaster builds, unit tests pass, failure down to linting

The important part of this patch seems to be missing! It says it's adding AArch32 assembly parsing for some new instructions, but I don't see any new Tablegen work that adds the instructions. New mnemonics are added to a couple of exclusion lists in ARMAsmParser, but there's no code in here that makes the mnemonics actually exist in the first place, or that says what their operands should be or what their encodings are – just that small C++ tweak and a load of tests. Where's the rest of it?

The important part of this patch seems to be missing! It says it's adding AArch32 assembly parsing for some new instructions, but I don't see any new Tablegen work that adds the instructions. New mnemonics are added to a couple of exclusion lists in ARMAsmParser, but there's no code in here that makes the mnemonics actually exist in the first place, or that says what their operands should be or what their encodings are – just that small C++ tweak and a load of tests. Where's the rest of it?

Apologies, I submitted multiple patches for review at once. The assembly support is ready to land and is found in [1], so I suppose I should rename this patch to "AArch32 Assembly Parser Support" or some variant thereof. What do you think?

[1] https://reviews.llvm.org/D77872

I see. In that case I suppose the simplest thing to do is just to explain that in the commit message, along the lines of "D77872 has already added the MC representations of the instructions so that they can be used in code gen; this patch fills in the details needed to make assembly parsing work, and adds tests for asm and disasm'.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
6335

Now I look closely, I'm slightly confused about why all of these instructions needed to be added to this if statement. I thought this if statement was supposed to be a list of exceptions to the general rule that a mnemonic ending in a known condition-code string like "eq" or "ne" or "lt" should be broken up into a shorter mnemonic and a conditional suffix. Hence, for example, "wls" is on the list so that it doesn't get misparsed as "w" with condition "ls".

Mind you, I'm also slightly confused about what some of the existing mnemonics in this list are here for. But what specific thing fails if you remove this hunk of the diff?

LukeGeeson marked an inline comment as done.Apr 20 2020, 8:39 AM
LukeGeeson added inline comments.
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
6335

Nothing fails when removing this hunk and running tests (check-all, llvm/test, clang/test).

I agree with you in principle, but I suppose here I'm appealing to the age old adage of 'we did it this way here'

I think we should keep it, and log a separate issue for refactoring this statement in a different issue

LukeGeeson retitled this revision from [AArch32] Armv8.6a Matrix Mul Assembly to [AArch32] Armv8.6a Matrix Mul Assembly Parser Support.
LukeGeeson edited the summary of this revision. (Show Details)
  • Updated Commit Message
  • Updated Commit Title
simon_tatham added inline comments.Apr 21 2020, 5:56 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
6335

I'm inclined to take the opposite view: if we don't add this hunk now, then if we later find something that breaks as a result, we'll know more about why it had to be here!

LukeGeeson marked an inline comment as done.
  • removed hunk
  • uploaded the right patch this time... (hunk is removed)
LukeGeeson added inline comments.Apr 21 2020, 7:15 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
6335

Very well, I shall remove this hunk

simon_tatham accepted this revision.Apr 21 2020, 7:53 AM
This revision is now accepted and ready to land.Apr 21 2020, 7:53 AM
This revision was automatically updated to reflect the committed changes.