[mlir][ArmSVE][RFC] Add an ArmSVE dialect

[jsetoain]

Summary

This revision starts an Arm-specific ArmSVE dialect discussed in the discourse RFC thread:

https://llvm.discourse.group/t/rfc-vector-dialects-neon-and-sve/2284

[mehdi_amini]

mlir/include/mlir/Dialect/SVE/SVE.td

115 ↗

(On Diff #307898)

Nice Ascii art :)

I suspect we need some markdown tweak to make sure it displays well after HTML conversion.

mlir/include/mlir/Dialect/SVE/SVEDialect.h

90 ↗

(On Diff #307898)

Can the types be generated from TableGen? https://mlir.llvm.org/docs/OpDefinitions/#type-definitions
(@rriddle may be able to advise here)

mlir/lib/Target/LLVMIR/LLVMSVEIntr.cpp

63 ↗

(On Diff #307898)

I don't understand why we need a specific conversion here. This concerns me, I see that we have this for "mlir-to-nvvmir" and "avx512-mlir-to-llvmir" but I don't understand why it is desirable. I'd expect the export to LLVM IR to be self-contained and not have many entry-points which don't compose with each others.

This looks to me that this would better be implemented using an op interface or similar mechanism instead.

You're not responsible for setting it up this way, but I'd still like to head from @ftynse / @rriddle on this.

[mehdi_amini]

This is really nice to see this, thanks for the contribution!

[rriddle]

mlir/include/mlir/Dialect/SVE/SVE.td

25 ↗

(On Diff #307827)

Apologies for the terse comments, added proper discussion to the RFC. My thoughts basically echo those of Mehdi, in that if we have something targeting a specific target we should ideally have that in the name similarly to how LLVM intrinsics work.

155 ↗

(On Diff #307827)

Is it possible to write the documentation of this operation in a way that can stand on it's own? I see the lowering to a specific llvm intrinsic as an implementation detail of the lowering. I'd prefer to avoid baking a specific lowering into the semantics of an operation. If we bake in the semantics such that we have "This op matches 1-1 with this llvm intrinsic", the dialect is basically just an extension of the LLVM dialect.

[jsetoain]

mlir/include/mlir/Dialect/SVE/SVE.td

25 ↗

(On Diff #307827)

Don't worry, I took no offence, dry language is fine by me but I wasn't sure what the suggestion was. It's clear now, let's solve this in the RFC :-)

115 ↗

(On Diff #307898)

Alternatively, I can remove the ASCII art. It's not in the original ISA documentation, but I find these "visual" explanations clearer than the formal specification, easier to notice the typical pitfalls with scalable vectors. Would that work as a quick and easy fix?

[jsetoain]

Fixed issue with doc formating.
Changed name of vector scale size operation to a better suited one, fixed documentation accordingly.

[jsetoain]

mlir/include/mlir/Dialect/SVE/SVE.td

155 ↗

(On Diff #307827)

Absolutely. I guess I was being lazy, my apologies. I hope changing the name and focusing on function instead of lowering target will avoid the confusion.

[mehdi_amini]

mlir/include/mlir/Dialect/SVE/SVE.td

115 ↗

(On Diff #307898)

ASCII art is fine, I was just pointing out that we need to surround it in-between something like <pre> / </pre> for it to properly display on the website later.

[jsetoain]

Switching to ODS for type generation

[jsetoain]

mlir/include/mlir/Dialect/SVE/SVEDialect.h

90 ↗

(On Diff #307898)

As it turns out, they can (I quite like it). Is this a new thing? I completely missed it and I noticed nobody else is using it. I've noticed something a bit odd, I still need to do the def SVE_ScalableVectorType : DialectType<SVE_Dialect, thing, but I don't see why TypeDef class can't do that automatically. Am I misusing it?

[mehdi_amini]

mlir/include/mlir/Dialect/SVE/SVEDialect.h

90 ↗

(On Diff #307898)

It is fairly recent (2 months maybe?)

I don't know for the other question, @rriddle will be able to help here.

[jsetoain]

Make operations signless.
Rename dialect and move it to a different namespace.

[nicolasvasilache]

Once my comments are addressed I am happy to land this.
I think the additional layer of tablegen discussed in https://llvm.discourse.group/t/exposing-llvm-ops-to-the-mlir-type-system/2290/11 and having everything in the LLVM dialect is a cross-cutting change that should be done separately and unified across all 3 dialects (AVX512, ArmNeon, ArmSVE).

mlir/include/mlir/Conversion/Passes.td

386

@aartbik improved the conversion to drop one-off passes.
See the changes to https://reviews.llvm.org/D92171 after I rebased on those.
Basically the ConvertVectorToLLVMPass gains a target-specific bit and conditionally includes the dependent dialects as well as the relevant conversion patterns.

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td

107

Not sure what the proper source of truth is here but in the Neon dialect I just copied a part of the description and referred to https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVEDialect.h

13

To silence the lint warnings, please spell out the whole path using _ in place of /.
e.g. MLIR_DIALECT_TARGET_ARMSVE_ARMSVEDIALECT_H

[nicolasvasilache]

mlir/include/mlir/Dialect/SVE/SVE.td

155 ↗

(On Diff #307827)

@rriddle see https://llvm.discourse.group/t/exposing-llvm-ops-to-the-mlir-type-system/2290/11, ideally we would only have a single dialect and an extra layer of tablegen but that should be done separately.

mlir/lib/Target/LLVMIR/LLVMSVEIntr.cpp

63 ↗

(On Diff #307898)

As usual with MLIR, this is on-demand and the avx512 use case was not justification enough.
We should now layer this better indeed but this is a cross cutting change that is better done separately.
I would not condition landing this on that particular change.

[nicolasvasilache]

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td

151

Let's make everything signless at this level, you could write something along the lines of
signless integer that the op interprets as signed/ unsigned as appropriate.

[mehdi_amini]

mlir/lib/Target/LLVMIR/LLVMSVEIntr.cpp

63 ↗

(On Diff #307898)

Are you committing to this refactoring? If you'd like to land this before it then I'd like to see a TODO in the code and a commitment from someone to make it happen "soon".

[jsetoain]

Fixed lint problems.
Improved documentation.
Merged with D92614, doesn't require it's own lowering pass anymore.

[jsetoain]

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td

107

I used the ArmARM, but I've switched to a source similar to yours. I think it's clearer that way.

151

Are you proposing to get rid of the distinction between smmla & ummla or is it just about the wording? If it's the former, I'm not sure how I'd go about differentiating between both cases if we come from signless MLIR. If it's the later, let me know if this works or you want me to make it clearer.

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVEDialect.h

13

I've fixed this one and the other one. Apologies for the overlook.

[aartbik]

Nice to see all vector dialects coming together!

mlir/include/mlir/Conversion/Passes.td

414–415

SVE -> ArmSVE

417

note that Nicolas made changes here too; race condition on rebasing :-)

425

rather not a default of true here ;-)

[nicolasvasilache]

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td

151

Just the wording, in line with this: https://llvm.discourse.group/t/rfc-vector-dialects-neon-and-sve/2284/12.
For smmla you'd write signless integer that the op interprets as signed.
For ummla you'd write signless integer that the op interprets as unsigned.

[jsetoain]

Fixed default value for enabling ArmSVE extensions in vector

[jsetoain]

Improve documentation around signedness of operations.

[jsetoain]

mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td

151

Yes, I saw that. Hopefully this wording is clearer.

[jsetoain]

mlir/include/mlir/Conversion/Passes.td

417

I noticed that while I was following your patch for AVX512. Unless there's a canonical way to deal with this, I'll take silver medal and deal with it once D92171 has landed :-)

[jsetoain]

Rebased on top of D92171
Dialect syntax homogenized with ArmNeon dialect
Naming convention aligned with ArmNeon dialect
File location aligned with ArmNeon dialect
Conversion code aligned with ArmNeon dialect
Improved type checking
Added extra scalable vector type traits

[aartbik]

LGTM, but leaving the approval to River since he added some blocking feedback earlier, so I want to make sure he feels everything has been addressed

mlir/test/Conversion/ArmSVEToLLVM/convert-to-llvm.mlir

8

putting { on previous line seems a bit more consistent with most of our tests

[jsetoain]

Align test syntax style with common practise

[rriddle]

mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td

82 ↗

(On Diff #311356)

nit: Can you use ShapedType::isDynamic here instead?

90 ↗

(On Diff #311356)

nit: Spell out auto here.

135 ↗

(On Diff #311356)

Is there any sort of ordering to these ops?

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVMPass.cpp

46 ↗

(On Diff #311356)

nit: The order here is different from the one used elsewhere.

mlir/lib/Dialect/ArmSVE/IR/ArmSVEDialect.cpp

41 ↗

(On Diff #311356)

Don't use namespaces for method qualification, use arm_sve:: instead (like above).

mlir/lib/Target/LLVMIR/LLVMArmSVEIntr.cpp

40

Only classes should go in anonymous namespaces, methods should be static and at the top level.

[jsetoain]

Reorder operations and initializations alphabetically
Removed useless code
Clean up unnecessary 'auto' declartion
Fixed namespace usage

[jsetoain]

mlir/include/mlir/Dialect/ArmSVE/ArmSVE.td

82 ↗

(On Diff #311356)

Well... that was embarrassing :">

135 ↗

(On Diff #311356)

I've switched to alphabetical

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVMPass.cpp

46 ↗

(On Diff #311356)

I've changed the others to leave all the vector extensions in alphabetical order

[jsetoain]

I don't have commit access to the repository. Please, commit when ready.