This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add vector.scalable.insert/extract ops
ClosedPublic

Authored by jsetoain on Jun 15 2022, 9:47 AM.

Details

Summary

These new operations match the semantics of
llvm.experimental.vector.insert and llvm.experimental.vector.extract.

vector.scalable.insert and vector.scalable.extract allow,
respectively, insert vectors into scalable vectors, and extract vectors
from scalable vectors.

The discussion about the inclusion of these operations is here:
https://discourse.llvm.org/t/rfc-interfacing-between-fixed-length-and-scalable-vectors-for-vls-vector-code-on-scalable-vector-architectures

Diff Detail

Event Timeline

jsetoain created this revision.Jun 15 2022, 9:47 AM
jsetoain updated this revision to Diff 437228.Jun 15 2022, 10:03 AM

Remove comments

jsetoain updated this revision to Diff 437533.Jun 16 2022, 7:34 AM

Changing the name and adjusting the semantics of insert/extract ops

jsetoain retitled this revision from [mlir][vector] Add vector.insert_1d/extract_1d ops to [mlir][vector] Add vector.scalable.insert/extract ops.Jun 16 2022, 7:35 AM
jsetoain edited the summary of this revision. (Show Details)
jsetoain published this revision for review.Jun 16 2022, 12:38 PM

Thanks, Javier! LGTM. Some minor comments.

mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
782–783

Not sure I'm getting this properly, how could a vector be rank-1 and n-D at the same time?

796

I'm assuming this will be aligned with the new LLVM doc, whatever that ends up being :)

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

We should bail out if the vector has multiple dimensions, right?

mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
1835

Don't need to capture TMP?

jsetoain updated this revision to Diff 440282.Jun 27 2022, 9:50 AM
jsetoain marked an inline comment as done.

Answer comments and rebase on main

mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
782–783

Throughout the documentation, we use "rank" and "dimensionality" interchangeably, but that is incorrect. This: vector<3xf32> is a 3-D vector; this: vector<2x3x4xf32> is a rank-3 vector. I have thought about this wording quite a lot while I was writing it, because the meaning is nor respected everywhere (comments and documentation). To prevent confusion, I'd be happy to try to avoid this language entirely, but it is an issue in the documentation.

796

I wrote the documentation here in far more detail than LLVM's LangRef, but I'm saying the same thing. If you think I should just respect whatever LLVM LangRef says and avoid clarifications, I'm happy to simplify this :-)

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

This should be prevented by the operand constraints in the operation definition:

Arguments<(ins VectorOfRank<[1]>:$source,

ScalableVectorOfRank<[1]>:$dest,
I64Attr:$pos)>,

Or is there a reason to verify it inside the lowering as well?

dcaballe added inline comments.Jun 27 2022, 11:29 AM
mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
782–783

If I'm not mistaken, in MLIR we would say that vector<3xf32> is a 1-D vector with shape or vector size = {3}. vector<2x3x4xf32> would be a 3-D vector (or vector with rank = 3) with shape or vector size = {2x3x4}. I would stick to that terminology for "consistency". Where did you see this being inconsistent?

796

It looks good! It's fine as long as they end up being aligned. I just mentioned that since the doc in LLVM is being rewritten.

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

Oh, that should be good enough for now. Thanks!

@jsetoain thanks for working on this! As hinted on IREE's Discord, I'm starting to look at scalable vectors myself and this would really help us :)

I've left a few minor comments, but otherwise LGTM. I'll let somebody more experienced to make this formal though :)

mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
782–783

I don't have a specific example, but agree with @jsetoain here - the nomenclature is a bit confusing and I wouldn't mind it being clarified. I can post a question on Discourse and propose something unless you have some other suggestion? Javier's documentation makes sense to me as is, though to me vectors have no rank 😂 . That's a property of tensors or matrices. But "rank-1" is intuitive enough. Ultimately, we should keep this consistent with what's used elsewhere in MLIR. I'm just not sure what that would be. @dcaballe, wdyt?

Some refs found online:

Note: The rank of a tensor is not the same as the rank of a matrix. The rank of a tensor is the number of indices required to uniquely select each element of the tensor. Rank is also known as "order", "degree", or "ndims."

The rank R of a tensor is independent of the number of dimensions N of the underlying space.

796

[nit] I would add a note that this is meant to be kept in sync/consistent with the relevant ops from LangRef. WDYT?

mlir/test/Dialect/Vector/invalid.mlir
1620

How about a similar test when the source vector is scalable?

mlir/test/Dialect/Vector/ops.mlir
792

Could you add a test for non-zero starting index?

awarzynski added inline comments.Oct 12 2022, 8:37 AM
mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
782–783

I brought this up on Discourse. That helped me realize that we are confusing the type and value domains. And that @dcaballe was right and I was wrong :)

So, we need to make sure that we are always referring to the indexing space here, so

  • vector<3xf32> is a rank-1 vector
  • vector<2x3x4xf32> is a rank-3 vector

Now, the "vector" dialect introduces n-D vectors. Using their terminology we would have:

  • vector<3xf32> is a 1-D vector
  • vector<2x3x4xf32> is a 3-D vector

@jsetoain

This: vector<3xf32> is a 3-D vector

In this statement you are referring to the value domain :)

I'd be happy to try to avoid this language entirely

If we accept the TF definition of a tensor/vector rank, then this document is starting to make sense to me. WDYT?

I thought this was already in! What is missing here? Only some doc fixes? Do you need any help with that, @jsetoain?

jsetoain updated this revision to Diff 469999.Oct 23 2022, 12:01 PM
jsetoain marked 11 inline comments as done.

Address comments

mlir/test/Dialect/Vector/invalid.mlir
1620

I can certainly add that, but we'd be testing the exact same code. I don't have a preference one way or another, but if you think it's worth the extra tests, happy to oblige :-)

I thought this was already in! What is missing here? Only some doc fixes? Do you need any help with that, @jsetoain?

Thank you for offering, Diego, it's greatly appreciated :-) Apologies for the delay, this should get things back in motion!

dcaballe accepted this revision.Oct 25 2022, 12:04 AM

Thanks, @jsetoain! LGTM. Please, wait for @awarzynski's approval/reply back.

This revision is now accepted and ready to land.Oct 25 2022, 12:04 AM
awarzynski accepted this revision.Oct 27 2022, 1:53 PM

LGTM, thanks for the updates!

jsetoain updated this revision to Diff 473905.Nov 8 2022, 12:50 AM

Rebase on main

This revision was landed with ongoing or failed builds.Nov 8 2022, 12:52 AM
This revision was automatically updated to reflect the committed changes.