Page MenuHomePhabricator

[mlir][vector] Add vector.scalable.insert/extract ops
Needs ReviewPublic

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!