This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ArmSME] Dialect and Intrinsic Op Definition
ClosedPublic

Authored by WanderAway on Jun 13 2023, 6:44 PM.

Details

Summary

This patch creates the ArmSME dialect, and provides the intrinsic op definition necessary for lowering to LLVM IR.

This will cover most instructions interacting with the ZA tile register, not covering SME2 instructions.

Source: https://developer.arm.com/documentation/ddi0616/latest

Diff Detail

Event Timeline

WanderAway created this revision.Jun 13 2023, 6:44 PM
Herald added a project: Restricted Project. · View Herald Transcript
WanderAway requested review of this revision.Jun 13 2023, 6:44 PM

Remove unneeded type constraints

Matt added a subscriber: Matt.Jun 13 2023, 7:44 PM
tblah added a subscriber: tblah.Jun 14 2023, 2:08 AM

Thanks for the patch Frank this is great. I've left some comments, mostly minor but generally LGTM

mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
10–11

nit: This file defines the ArmSME dialect and contains intrinsic ops to lower to LLVM IR.

44

nit: ArmSME_IntrOp is sufficient I think given overloading is optional.

56

I prefer the naming format used in the AMX dialect e.g. LLVM_x86_amx_tilezero. It makes it obvious these are LLVM intrinsics and if this were LLVM_aarch64_sme_zero it would be found when grepping on aarch64.sme.zero (alongside the LLVM intrinsic and tests). What do you think?

56

this is the default and can be removed.

57
87

tile slice?

89–92

may as well add b/h/q intrinsics as well (and below for stores)

mlir/test/Target/LLVMIR/arm-sme.mlir
4

should we follow LLVM naming nxv2f64 given this is scalable?

19–54

missing tests for outer product and subtract intrinsics

20

nit: move all types to next line? E.g.

"arm_sme.intr.mopa"(%c0, %v2i1, %v2i1, %v2f64, %v2f64) :
  (i32, vector<[2]xi1>, vector<[2]xi1>, vector<[2]xf64>, vector<[2]xf64>) -> ()

Hi Frank, thanks for working on this! Also LGTM modulo some minor comments.

Does this cover all SME intrinsics? In particular, I don't see any SME 2. That's totally fine, but would be good to clarify the scope in the summary.

mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
34

This spec doesn't cover all the SME instructions. It would be good to add a link to https://developer.arm.com/documentation/ddi0602/2023-03/SME-Instructions?lang=en as well.

57

The tiles in SME aren't registers. And also aren't real tiles :) I've seen people using the term "virtual tile", which IMHO makes a lot of sense.

65

Why aren't the input arguments limited to "supported" vector sizes like predicates?

def Predicate : ScalableVectorOfLengthAndType<[16, 8, 4, 2], [I1]>;

It's something that we can change later. I'm just curious about the rationale.

Also, what happens when invalid predicate/input size is used?

86
99–100
mlir/test/Target/LLVMIR/arm-sme.mlir
4

should we follow LLVM naming nxv2f64 given this is scalable?

+1

Also, could you split this into multiple functions? Perhaps a function per intrinsic group. I'd also further split into integer vs FP.

WanderAway edited the summary of this revision. (Show Details)Jun 14 2023, 7:23 AM
WanderAway set the repository for this revision to rG LLVM Github Monorepo.
WanderAway marked 13 inline comments as done.

Addressed comments

mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
10–11

Oops, fixed.

44

Makes sense.

56

Makes sense. Changed.

57

mask

Oops too much copy pasting

I've seen people using the term "virtual tile"

Sounds good.

65

Why aren't the input arguments limited to "supported" vector sizes like predicates?

Honestly I can't remember, there was a reason at some point, but right now I think it makes sense to add a constraint for it.

what happens when invalid predicate/input size is used?

If the predicate and vector sizes mismatch, then it violates the intrinsic signature, and emit a error during LLVM verifier. However there are currently no checks in place to make sure that the vector unit length is 128b (i.e. <vscalex2xf32> is a valid operand type). Right now there are no built-in constraints to check for something like this (although we could add one), and I suspect it will fail during ISel.

c-rhodes added inline comments.Jun 14 2023, 9:44 AM
mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
33–34

Please could we keep the previous link to the spec as well, that contains general information about the SME architecture like ZA layout etc.

47
47–48

nit: fix indentation

51

nit: add whitespace around concat operator

52

and here

111–115

nit: please could we order from smallest to largest types (i.e define b first and q last) to be consistent with other parts of the codebase

mlir/test/Target/LLVMIR/arm-sme.mlir
2

add -split-input-file and separate each test with // ----- to run in parallel?

4

missing CHECK-LABELs

Fixed more comments.

WanderAway marked 10 inline comments as done.Jun 14 2023, 10:41 AM
c-rhodes accepted this revision.Jun 14 2023, 12:59 PM

Just one last comment but otherwise LGTM, thanks!

mlir/lib/Dialect/ArmSME/IR/ArmSME.cpp
16–21

I think we can drop these includes?

This revision is now accepted and ready to land.Jun 14 2023, 12:59 PM

Removed unnecessary header files.

WanderAway marked an inline comment as done.Jun 14 2023, 1:10 PM
awarzynski accepted this revision.Jun 14 2023, 1:29 PM

LGTM, thank you for addressing my comments!

Feel free to ignore my final nit.

mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
43–45

[nit] I believe that we will be using these for Ops other than outer product as well. I would use a more generic name instead (e.g. SVEPredicate/SVEVector or SSVEPredicate/SSVEVector to be more precise).

WanderAway marked 2 inline comments as done.Jun 14 2023, 1:37 PM
WanderAway added inline comments.
mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td
43–45

I believe that we will be using these for Ops other than outer product as well. I would use a more generic name instead (e.g. SVEPredicate/SVEVector or SSVEPredicate/SSVEVector to be more precise).

The only issue I see here is that different instructions may have different types supported (such as load/stores and MOPs defined here), so it may be too much of a generalization to rename these to SVE* or SSVE* at the current moment. I think it may be a good idea to rename these to something appropriate whenever we add more to this file.

This revision was automatically updated to reflect the committed changes.