This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [VectorOps] Add 'vector.flat_transpose' operation
ClosedPublic

Authored by aartbik on May 21 2020, 6:13 PM.

Details

Summary

Provides a representation of the linearized LLVM instrinsic.
With tests and lowering implementation to LLVM IR dialect.
Prepares better lowering for 2-D vector.transpose.

Diff Detail

Event Timeline

aartbik created this revision.May 21 2020, 6:13 PM
aartbik updated this revision to Diff 265660.May 21 2020, 7:01 PM

fixed typo

dcaballe requested changes to this revision.May 22 2020, 9:12 AM

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?

mlir/include/mlir/Dialect/Vector/VectorOps.td
1552

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?

1565

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.

1571

Could you also briefly describe how to represent the multi-dim version of this in the Vector dialect?

This revision now requires changes to proceed.May 22 2020, 9:12 AM
aartbik marked 4 inline comments as done.May 26 2020, 10:19 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
1552

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

1565

I moved the comment around a bit, see if that's better (note however that we have more instances of ops close to intrinsics)

1571

Added more comments

aartbik updated this revision to Diff 266276.May 26 2020, 10:54 AM
aartbik marked an inline comment as done.

clarified that certain ops ('matrix') are only there for progressive lowering

aartbik updated this revision to Diff 266280.May 26 2020, 11:24 AM

fixed typo in new comments

ftynse accepted this revision.May 27 2020, 7:32 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
1552

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.

1590

You can omit = ? lines, these are the default anyway.

1591

Nit: do we really need parenthesis around type($matrix) ?

mlir/test/Dialect/Vector/invalid.mlir
1129

Nit: we don't have to verify auto-generated messages

dcaballe accepted this revision.May 27 2020, 8:46 AM

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

mlir/include/mlir/Dialect/Vector/VectorOps.td
1552

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.

This revision is now accepted and ready to land.May 27 2020, 8:46 AM
aartbik marked 8 inline comments as done.May 27 2020, 10:20 AM
aartbik added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
1552

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.

1590

Removed, for matrix_multiply too

1591

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.

mlir/test/Dialect/Vector/invalid.mlir
1129

Fair enough. I wanted something here. I think we should actually verify that rows x columns is valid, also for the matrix multiply.

aartbik updated this revision to Diff 266598.May 27 2020, 10:36 AM
aartbik marked 4 inline comments as done.

addressed comments

aartbik retitled this revision from [mlir] [VectorOps] Add 'vector.matrix_transpose' operation to [mlir] [VectorOps] Add 'vector.flat_transpose' operation.May 27 2020, 10:48 AM
This revision was automatically updated to reflect the committed changes.