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

Fri May 22 09:38:33 PDT 2020

dcaballe requested changes to this revision.
dcaballe added a comment.
This revision now requires changes to proceed.

Thanks! It looks good to me in general. I just added some comments about making it a bit more generic.
For my understanding, how do we plan to lower to these vector matrix multiply and transpose? From a vector contraction?

================
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",
----------------
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`?

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80419