Page MenuHomePhabricator

[mlir][RFC] Add scalable dimensions to VectorType
Needs ReviewPublic

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.vector_scale

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
905

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
660

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
297

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

320

Same here.

344

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.Wed, Nov 10, 3:44 AM

Rebase on main

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

Rebase on main

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

Rebase on main

rriddle added inline comments.Tue, Nov 30, 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
2404–2405

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

mlir/include/mlir/IR/BuiltinTypes.td
941–942

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
295–298
318–319

Same here.

342–343

and here.

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

Address review comments

jsetoain edited the summary of this revision. (Show Details)Wed, Dec 1, 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.Fri, Dec 3, 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.Fri, Dec 3, 3:39 PM
jsetoain edited the summary of this revision. (Show Details)
mlir/include/mlir/Dialect/Vector/VectorOps.td
2399

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

2404–2405

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
907

And this...

941–942

Not sure what happened there. Good catch, thanks!

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

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