[PATCH] D80419: [mlir] [VectorOps] Add 'vector.matrix_transpose' operation

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 4 inline comments as done.
aartbik added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1533
+/// MLIR vectors. This is the counterpart of llvm.matrix.transpose in MLIR.
+def Vector_MatrixTransposeOp : Vector_Op<"matrix_transpose", [NoSideEffect,
+  PredOpTrait<"source operand and result have same element type",
----------------
dcaballe wrote:
> Since the Vector dialect supports multi-dim vectors, I think we should be explicit about the `flattened` nature of this op in the name class and the mnemonic. Otherwise, it would be surprising that `vector.matrix_transpose` only accepts 1D vectors. What about something like `FlatMatrixTransposeOp`/`flat_matrix_transpose`?
Note that we already have an instance of an op needed for progressive lowering and type change (see line 1467 that starts this section). The "matrix" in the name gives the special meaning. I a

   regular                       mid-level
--------------------+-----------------
vector.contract    ->  vector.matrix_multiply

vector.transpose  ->  vector_matrix_transpose 

So hopefully this is sufficient. If you insist, however, we should rename them both into flat_matrix..




================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1546
+  let description = [{
+    This is the counterpart of llvm.matrix.transpose in MLIR. It serves
+    the purposes of more progressive lowering and localized type conversion.
----------------
dcaballe wrote:
> Since this is the Vector dialect and should be backend independent, I would suggest making the description more generic so that other backends can also use it. We can talk about the `llvm.matrix.transpose` at the end just as one potential lowering example.
 I moved the comment around a bit, see if that's better (note however that we have more instances of ops close to intrinsics)


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1552
+    transposed matrix in flattened form in 'res'.
+
+    Also see:
----------------
dcaballe wrote:
> Could you also briefly describe how to represent the multi-dim version of this in the Vector dialect?
Added more comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80419/new/

https://reviews.llvm.org/D80419