This is an archive of the discontinued LLVM Phabricator instance.

[mlir][RFC] Add scalable dimensions to VectorType
ClosedPublic

Authored by jsetoain on Oct 14 2021, 9:51 AM.

Details

Summary
With VectorType supporting scalable dimensions, we don't need many of
the operations currently present in ArmSVE, like mask generation and
basic arithmetic instructions. Therefore, this patch also gets
rid of those.

Having built-in scalable vector support also simplifies the lowering of
scalable vector dialects down to LLVMIR.

Scalable dimensions are indicated with the scalable dimensions
between square brackets:

        vector<[4]xf32>

Is a scalable vector of 4 single precission floating point elements.

More generally, a VectorType can have a set of fixed-length dimensions
followed by a set of scalable dimensions:

        vector<2x[4x4]xf32>

Is a vector with 2 scalable 4x4 vectors of single precission floating
point elements.

The scale of the scalable dimensions can be obtained with the Vector
operation:

        %vs = vector.vscale

This change is being discussed in the discourse RFC:

https://llvm.discourse.group/t/rfc-add-built-in-support-for-scalable-vector-types/4484

Differential Revision: https://reviews.llvm.org/D111819

Diff Detail

Event Timeline

jsetoain created this revision.Oct 14 2021, 9:51 AM
jsetoain published this revision for review.Oct 14 2021, 10:44 AM
jsetoain retitled this revision from [mlir] Make scalable vector type a built-in type to [mlir][RFC] Make scalable vector type a built-in type.
jsetoain edited the summary of this revision. (Show Details)
Matt added a subscriber: Matt.Oct 15 2021, 3:14 PM

This direction makes a lot of sense to me, if we want to avoid code dup between the upcoming vector specific dialects (SVE and RISC-V at the moment).
Since this touches "core", however, I hope others chime in too.

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
449

period at end

450

would IsScalableVectorType be a bit more consistent with naming?
(in sentence you would put scalable at the end, but since we use "ScalableVectorType" as typename this seems a bit better)

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1123 ↗(On Diff #379753)

compile-time

1125 ↗(On Diff #379753)

comma, no : Also, is this not better an e.g.?
Or just:

For example, ....

mlir/include/mlir/IR/BuiltinTypes.td
907

Perhaps you can add some text calling out that < > is fixed length and << >> is scalable?
Just because it is a new syntax that we have to get used to ;-)

921

period at end

924

period at end

mlir/include/mlir/IR/OpBase.td
651

I wanted to say period at end, but I see that is not really the style in this file

rriddle requested changes to this revision.Oct 21 2021, 1:02 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1118–1120 ↗(On Diff #379753)

Why would this be here and not in say, the vector dialect?

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
554–555

Drop else after return.

568–570
587–588

Drop else after return.

mlir/lib/IR/BuiltinTypes.cpp
298

Use cast if you aren't checking the result, dyn_cast> can return null.

322

Same here.

346

And here, and others.

mlir/lib/Target/LLVMIR/TypeToLLVM.cpp
147–148

Drop else after return.

This revision now requires changes to proceed.Oct 21 2021, 1:02 PM
jsetoain added inline comments.Oct 21 2021, 1:42 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1118–1120 ↗(On Diff #379753)

Indeed, my first instinct was to put it in Vector, but since this value is tightly coupled with the type itself, which is _not_ part of Vector, it feels a bit out of place there as well. I believe that the Vector type not being part of the Vector dialect is what creates the situation. As things are, if there's a place to put runtime constants, that's where this should go. Alternatively, if there's a way to express runtime properties of a type, say:

%0 = vector<<>>.scale : index

That also looks somewhat right (if a bit ugly). In any case, Standard or Vector, neither place looks better than the other to me, if you see clearly that this makes more sense in Vector, I can move it there and everything else works the same. This is something I actually wanted feedback about, thanks :-)

rriddle added inline comments.Oct 21 2021, 1:43 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1118–1120 ↗(On Diff #379753)

I don't think it fits well in standard though, given that the standard dialect is going away. It seems like a better home should be found for this.

jsetoain added inline comments.Oct 21 2021, 2:44 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
1118–1120 ↗(On Diff #379753)

Agree. I'll move it to Vector as a non-entirely-terrible option and, should a better place become apparent, we can reconsider in the future. It's not intertwined with anything else, it'd be a quite innocuous change. Thanks!

This direction makes a lot of sense to me, if we want to avoid code dup between the upcoming vector specific dialects (SVE and RISC-V at the moment).
Since this touches "core", however, I hope others chime in too.

My main concern is that the discussion on Discourse does not seem to have a conclusion right now.

This direction makes a lot of sense to me, if we want to avoid code dup between the upcoming vector specific dialects (SVE and RISC-V at the moment).
Since this touches "core", however, I hope others chime in too.

My main concern is that the discussion on Discourse does not seem to have a conclusion right now.

My apologies. We did have a conversation this week but I didn't update the RFC (I have now). Although some details are still in the air, the need for a built-in type is not in question. This is just the first, most basic implementation possible.

jsetoain updated this revision to Diff 384043.Nov 2 2021, 4:32 AM
jsetoain marked 16 inline comments as done.

Address reviewers' comments

jsetoain marked an inline comment as done.Nov 2 2021, 4:34 AM

I've addressed all the comments.

jsetoain edited the summary of this revision. (Show Details)Nov 2 2021, 4:41 AM
jsetoain updated this revision to Diff 384491.Nov 3 2021, 9:32 AM

Fixed formatting

jsetoain updated this revision to Diff 386113.Nov 10 2021, 3:44 AM

Rebase on main

jsetoain updated this revision to Diff 389149.Nov 23 2021, 3:35 AM

Rebase on main

jsetoain updated this revision to Diff 390086.Nov 26 2021, 9:28 AM

Rebase on main

rriddle added inline comments.Nov 30 2021, 4:12 PM
mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
450

The get here is a bit weird, why not something like isScalableVectorType?

mlir/include/mlir/Dialect/Vector/VectorOps.td
2388–2389

You could also drop the trailing type if you want, it can be inferred. (i.e. = "attr-dict")

mlir/include/mlir/IR/BuiltinTypes.td
936

Is this wrapped at 80 characters?

mlir/lib/Dialect/ArmSVE/IR/ArmSVEDialect.cpp
51
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
161

Why the extra spaces?

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
551–557
mlir/lib/IR/BuiltinTypes.cpp
296–299
320

Same here.

344

and here.

mlir/lib/Parser/TypeParser.cpp
454–456
jsetoain updated this revision to Diff 390963.Dec 1 2021, 2:39 AM
jsetoain marked 10 inline comments as done.

Address review comments

jsetoain edited the summary of this revision. (Show Details)Dec 1 2021, 2:45 AM

What's the next step on this? Seems like the RFC discussion got to a resolution right?

@nicolasvasilache @ftynse : can you chime in on the change in mlir/include/mlir/IR/BuiltinTypes.td ?

My apologies for the long delay, my biggest problem atm is I do not have a good mental model of how RVV and Arm SVE operate in detail.
I had started to read specs but it invariably gets pushed back on the stack as it is not high on my priority list ..

From a pure cleanup perspective, I generally like it.

From a composability perspective, I think I would prefer to have it spelled out as vector<<4>xf32> or vector<4*xf32> or vector<(4s)xf32>.
The rationale is that I think we still want to have n-D scalable vector types in MLIR to allow expressing a statically known number of 1-D scalable vectors that serves as an "unroll-and-jammed vector pack" vector<8x4*xf32>.
This would be more consistent with the design of the rest of the vector dialect.

One thing that is higher priority to me personally these days is that we are also exploring using the vector dialect as a programming model for GPUs.
In this context, vector<4x8x16*x32*xf32> would also make sense for us.

Bottom line, if we avoided anchoring on the current LLVM / HW implementation that only support 1-D scalable vectors and we made it future-proof in that direction, I am fine with proceeding.

jsetoain updated this revision to Diff 391762.Dec 3 2021, 3:38 PM

Generalize the concept of scalable dimensions to support use cases unrelated to scalable vectors

jsetoain retitled this revision from [mlir][RFC] Make scalable vector type a built-in type to [mlir][RFC] Add scalable dimensions to VectorType.Dec 3 2021, 3:39 PM
jsetoain edited the summary of this revision. (Show Details)
mlir/include/mlir/Dialect/Vector/VectorOps.td
2383

I've seen this, I'll take care of it together with any other necessary fix.

2388–2389

It feels a bit "naked", but it might be because I'm used to see it with the return type attached. We can give it a go and see what people think, if people don't care, going "concise" is my preferred option. Is there a "good practices" manual for dialect syntax? I can't find one.

mlir/include/mlir/IR/BuiltinTypes.td
909

And this...

936

Not sure what happened there. Good catch, thanks!

mlir/lib/Parser/TypeParser.cpp
454–456

Arg! That was embarrassing... Sorry about that!

Mostly LGTM, I added 3 areas of improvements.
Once these are addressed I'll happily accept.
Thanks for your hard work and patience!

mlir/include/mlir/Dialect/Vector/VectorOps.td
2374

Should we call this vector.vscale ?

2378

I would emphasize that this is for 1-D scalable vectors and that there is currently no way to extract the scale
for a >1-D scalable vectors.
This instruction may be extended in the future to take a position but I am unclear whether this is what we want atm.

I think the global vs local property of vscale should also be discussed here.

I'd maybe even go as far as spelling it vector.scale.global in the future?

Edit: as I read deeper through the PR, I am now unclear whether vector<[2x8]xf32> is the same as vector<[2]x[8]xf32> ?

I think vector<[2x8]xf32> would make sense for SVE in MLIR (and would then get flattened to 1-D going through LLVM).
In the future we may also want vector<[2]x[8]xf32> for GPUs but this is not the same representation?
Is this what you have in mind ?

In any case, please propose a few wording changes to integrate the relevant parts of my comments and disregard/add a TODO for the others :)

mlir/include/mlir/IR/BuiltinTypes.h
323 ↗(On Diff #391762)

I fear this will prove annoying to use in practice ..

Could we go with unsigned numScalableDims?

Then you can just use APIS such as ArrayRef's shape.take_back(numScalableDims); and friends.

342 ↗(On Diff #391762)

this would get nicer with numScalableDims.

mlir/include/mlir/IR/BuiltinTypes.td
928

Now that I read this, I am unclear whether vector<[2x8]xf32> is the same as vector<[2]x[8]xf32>, I would think not and the latter form could be a future extension (if so, add a TODO)?
This really depends on whether you think you can make use of vector<[2x8]xf32> in MLIR instead of having to represent as vector<[16]xf32>; I claim you would have a bunch of nice use cases for this (coupled with the shape_cast op once properly extended).

mlir/test/Dialect/ArmSVE/legalize-for-llvm.mlir
1

I would expect to see a test file (somwhere in the builtin stuff) where you have both:

  • negative tests for various failure modes of misuses of scalable vectors (with appropriate error messages)
  • positive tests with multi-dim multi-scale vector (atm everything I see is 0-dim 1-scale only).

In a followup PR, I'd love to see a 1-dim, 2-scale version of the neon 2d dot (or something equivalent) and see it lower to unrolled LLVM.

Also signal boosting for @ThomasRaoux @dcaballe @springerm ; no need to review but be aware that this is coming.

rriddle added inline comments.Dec 6 2021, 12:35 AM
mlir/include/mlir/IR/BuiltinTypes.td
937

Can we use Optional here instead? -1 is a bit magic.

953–954

Why not just isScalable? The naming here is a bit weird.

mlir/lib/IR/AsmPrinter.cpp
1945

Please cache the end iterator to avoid recomputing it every iteration.

mlir/lib/Parser/TypeParser.cpp
525

Looks like this is missing test coverage.

jsetoain added inline comments.Dec 6 2021, 4:11 AM
mlir/include/mlir/IR/BuiltinTypes.td
928

It is, indeed, very much not the same. I find it useful to think about something like [2x8] as a series of 2x8 blocks, one after another. Therefore, even though they would have the same memory requirements, [2x8], [8x2], [4x4], and [16] can represent different data arrangements when you're loading your data from memory. From that point of view, [2]x[8] can't be the same as [2x8] even if the scale for both dimensions is the same. In fact, I don't think something like [2]x[8] makes sense in the context of scalable vectors. For GPU thread blocks, the situation is different. I'm not involved with that work so I can't come up with anything on the spot, but I intuit it could have potentially useful cases. As this work progresses, I suspect we will need to come back to it.

jsetoain updated this revision to Diff 393535.Dec 10 2021, 10:42 AM
jsetoain marked 9 inline comments as done.

Addressed latest round of reviews

I've also moved a bunch of tests around. Scalable vector tests that are not SVE-specific have been moved either to Arithmetic or Vector, depending on their nature.

mlir/include/mlir/Dialect/Vector/VectorOps.td
2378

That's not exactly right. You can have a 2D scalable vector, and vscale represents its multiplicity, but you can't have a 2D scalable vector with two different scales (which we might want to have for GPUs). As it is, we can't represent those yet, so I don't think we need to clarify that in the description. I've added the multi-dimensional multi-scale vector and local/global scale to a TODO for future reference.

mlir/include/mlir/IR/BuiltinTypes.td
937

Not sure how to use Optional for Types, this is the only way I found to provide a default value in a type builder. In any case, I've changed it to "numScalableDims" as suggested by Nicolas. It makes code a bit less awkward and conveniently replaces a arguably ugly "first dimension = -1" to a more semantically sensible "number of dimensions = 0". If you still find this unacceptable, I can look into adding an "Optional" equivalent for types.

mlir/test/Dialect/ArmSVE/legalize-for-llvm.mlir
1

RE follow-up PR, that was in my low priority TODO list, I'll move it to the main TODO, it should be a quick and easy change.

jsetoain edited the summary of this revision. (Show Details)Dec 10 2021, 10:45 AM
rriddle accepted this revision.Dec 10 2021, 7:27 PM

LG from my point-of-view, but also get an LGTM from Nicolas as well.

mlir/include/mlir/Dialect/Vector/VectorOps.td
2374

Can you move this into the documentation of the op? This seems useful to expose in the user facing docs.

2388–2389

I don't think we have a "good practices" manual, though that sounds useful.

2399

If a verifier isn't necessary, you can just ignore it.

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
31

nit: Prefer pre-increment unless you need post increment behavior.

38

Same here.

mlir/lib/IR/AsmPrinter.cpp
1945

Unresolved.

This revision is now accepted and ready to land.Dec 10 2021, 7:27 PM
jsetoain updated this revision to Diff 393961.Dec 13 2021, 10:39 AM
jsetoain marked 7 inline comments as done.

Address reviewer comments

Addressed comments.

Thanks for getting these through @jsetoain !

jsetoain updated this revision to Diff 394198.Dec 14 2021, 3:16 AM

Rebase on main

This revision was automatically updated to reflect the committed changes.
mlir/test/Dialect/ArmSVE/legalize-for-llvm.mlir