This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jsetoain on Nov 26 2020, 4:13 AM.

Details

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

Diff Detail

Event Timeline

jsetoain created this revision.Nov 26 2020, 4:13 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
jsetoain requested review of this revision.Nov 26 2020, 4:13 AM
jsetoain retitled this revision from [mlir][SVE] Add an SVE dialect to [mlir][SVE][RFC] Add an SVE dialect.Nov 26 2020, 4:13 AM
jsetoain added a reviewer: nicolasvasilache.

What discourse thread was this discussed in?

rriddle requested changes to this revision.Nov 26 2020, 4:21 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMSVE.td
54 ↗(On Diff #307827)

Remove commented out code.

mlir/include/mlir/Dialect/SVE/SVE.td
25 ↗(On Diff #307827)

SVE seems way too generic of a name for some seemingly specific to arm.

71 ↗(On Diff #307827)

I'd prefer you resolve these todos right now.

155 ↗(On Diff #307827)

I dont particularly like the idea of hard coding a higher level dialect specifically on what it would target in LLVM.

This revision now requires changes to proceed.Nov 26 2020, 4:21 AM

What discourse thread was this discussed in?

https://llvm.discourse.group/t/rfc-vector-dialects-neon-and-sve/2284 but manually creating http links and synchronizing at sub minute granularity is hard :)

@jsetoain could you please update your commit message with this link?

What discourse thread was this discussed in?

https://llvm.discourse.group/t/rfc-vector-dialects-neon-and-sve/2284 but manually creating http links and synchronizing at sub minute granularity is hard :)

@jsetoain could you please update your commit message with this link?

Thanks, I just assumed I missed the thread that discussed this.

jsetoain edited the summary of this revision. (Show Details)Nov 26 2020, 7:03 AM
jsetoain updated this revision to Diff 307876.EditedNov 26 2020, 8:24 AM

Change to comply with header guard naming style

jsetoain updated this revision to Diff 307878.Nov 26 2020, 8:30 AM

Remove commented code

jsetoain updated this revision to Diff 307898.Nov 26 2020, 9:28 AM
jsetoain marked an inline comment as done.

Added missing documentation for SVE intrinsics

jsetoain marked an inline comment as done.Nov 26 2020, 9:54 AM
jsetoain added inline comments.
mlir/include/mlir/Dialect/SVE/SVE.td
25 ↗(On Diff #307827)

Do you foresee a problem with this specific acronym or are you suggesting we should implement some sort of vendor-specific (or, more accurately, IP-designer-specific) naming convention for hardware-specific dialects? I'm happy to change the name if it makes more sense, but I'd need some guidance.

155 ↗(On Diff #307827)

I'm not sure I understand the issue. Would you mind to elaborate, please? Notice that, in order to generate scalable vector code, we need a primitive that provides, at run-time, the scale of the vector; this would feed into the step of the outer loop. LLVM provides llvm.vscale for this purpose, and it's conveniently architecture-independent. If there are better alternatives, I'm happy to consider them. If I'm missing something, please let me know. Thank you!

mehdi_amini added inline comments.Nov 26 2020, 4:27 PM
mlir/include/mlir/Dialect/SVE/SVE.td
25 ↗(On Diff #307827)

If this is not a vendor agnostic "scalable vector" abstraction but it is specific to the ARM SVE implementation, then what about naming this arm_sve as dialect name instead?

mehdi_amini added inline comments.Nov 26 2020, 4:27 PM
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.

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

rriddle added inline comments.Nov 27 2020, 12:49 AM
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 added inline comments.Nov 27 2020, 3:55 AM
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 updated this revision to Diff 308077.Nov 27 2020, 9:35 AM

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

jsetoain marked 3 inline comments as done.Nov 27 2020, 9:41 AM
jsetoain added inline comments.
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 added inline comments.Nov 27 2020, 11:09 AM
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 marked an inline comment as done.Nov 27 2020, 12:14 PM
jsetoain updated this revision to Diff 308101.Nov 27 2020, 12:16 PM

Switching to ODS for type generation

jsetoain marked an inline comment as done.Nov 27 2020, 12:23 PM
jsetoain added inline comments.
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?

jsetoain marked an inline comment as done.Nov 27 2020, 12:25 PM
mehdi_amini added inline comments.Nov 27 2020, 1:07 PM
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 updated this revision to Diff 309310.Dec 3 2020, 11:01 AM

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

jsetoain retitled this revision from [mlir][SVE][RFC] Add an SVE dialect to [mlir][ArmSVE][RFC] Add an SVE dialect.Dec 3 2020, 11:03 AM

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
375

@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
108

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
14

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

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.

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

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 added inline comments.Dec 7 2020, 1:20 PM
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 updated this revision to Diff 310040.Dec 7 2020, 3:28 PM

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

jsetoain marked 3 inline comments as done.Dec 7 2020, 3:36 PM
jsetoain added inline comments.
mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td
108

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

152

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
14

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

Nice to see all vector dialects coming together!

mlir/include/mlir/Conversion/Passes.td
399

SVE -> ArmSVE

405

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

419

rather not a default of true here ;-)

jsetoain marked 2 inline comments as done.Dec 8 2020, 1:03 AM
mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td
152

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 updated this revision to Diff 310120.Dec 8 2020, 2:57 AM

Fixed default value for enabling ArmSVE extensions in vector

jsetoain updated this revision to Diff 310123.Dec 8 2020, 3:15 AM
jsetoain marked 2 inline comments as done.

Improve documentation around signedness of operations.

jsetoain marked 2 inline comments as done.Dec 8 2020, 3:19 AM
jsetoain added inline comments.
mlir/include/mlir/Dialect/Target/ArmSVE/ArmSVE.td
152

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

jsetoain marked an inline comment as done.Dec 8 2020, 3:20 AM
jsetoain added inline comments.Dec 8 2020, 3:29 AM
mlir/include/mlir/Conversion/Passes.td
405

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 updated this revision to Diff 311298.Dec 11 2020, 12:11 PM

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

jsetoain marked an inline comment as done.Dec 11 2020, 12:11 PM
jsetoain marked 9 inline comments as done.Dec 11 2020, 12:20 PM
jsetoain retitled this revision from [mlir][ArmSVE][RFC] Add an SVE dialect to [mlir][ArmSVE][RFC] Add an ArmSVE dialect.Dec 11 2020, 12:24 PM
jsetoain edited the summary of this revision. (Show Details)

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 updated this revision to Diff 311356.Dec 11 2020, 6:58 PM

Align test syntax style with common practise

jsetoain marked an inline comment as done.Dec 11 2020, 6:58 PM
rriddle accepted this revision.Dec 14 2020, 10:41 AM
rriddle added inline comments.
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
37

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.

This revision is now accepted and ready to land.Dec 14 2020, 10:41 AM
jsetoain updated this revision to Diff 311675.Dec 14 2020, 12:47 PM

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

jsetoain marked 6 inline comments as done.Dec 14 2020, 12:55 PM
jsetoain added inline comments.
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
37

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

jsetoain marked 3 inline comments as done.Dec 14 2020, 12:55 PM

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

This revision was landed with ongoing or failed builds.Dec 14 2020, 1:35 PM
This revision was automatically updated to reflect the committed changes.