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

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 8 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:
> 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.
Ok, since requested by two now, I renamed this flat_tranpose since renaming later is a bit of a pain always (but I still feel my naming was closer the llvm conventions). I leave the renaming of the existing matrix_multiply to Nicolas then, since he has some other code invested in that already.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1590
+  }];
+  let verifier = ?;
+  let assemblyFormat = "$matrix attr-dict `:` `(` type($matrix) `)` `->` type($res)";
----------------
ftynse wrote:
> You can omit `= ?` lines, these are the default anyway.
Removed, for matrix_multiply too


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1591
+  let verifier = ?;
+  let assemblyFormat = "$matrix attr-dict `:` `(` type($matrix) `)` `->` type($res)";
+}
----------------
ftynse wrote:
> Nit: do we really need parenthesis around `type($matrix)` ? 
I felt it was consistent with the matrix multiply case. As is, I feel we are getting a bit inconsistent with our "to", "into" and "->". I removed it here though, since you requested it.


================
Comment at: mlir/test/Dialect/Vector/invalid.mlir:1129
+func @matrix_transpose_type_mismatch(%arg0: vector<16xf32>) {
+  // expected-error at +1 {{'vector.matrix_transpose' op failed to verify that source operand and result have same element type}}
+  %0 = vector.matrix_transpose %arg0 { rows = 4: i32, columns = 4: i32 } : (vector<16xf32>) -> vector<16xf64>
----------------
ftynse wrote:
> Nit: we don't have to verify auto-generated messages
Fair enough. I wanted something here. I think we should actually verify that rows x columns is valid, also for the matrix multiply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80419