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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe accepted this revision.
dcaballe added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM. You can rename both intrinsics in a separate commit.



================
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",
----------------
ftynse wrote:
> aartbik wrote:
> > 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..
> > 
> > 
> I tend to agree with @dcaballe: while `vector.matrix_multiply` on flat vectors makes sense more or less, `vector.matrix_transpose` on flat vectors and `vector.transpose` on nD vectors sounds the like opposite of what one would naturally assume. 
Yeah, I thing having `vector.flat_transpose` and `vector.flat_matrix_multiply` would be more clear. Feel free to change it in a separate commit, though. No problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80419