Page MenuHomePhabricator

[mlir][spirv] Add MatrixTimesMatrix operation

Authored by Hazem on Jun 26 2020, 11:35 AM.



Add MatrixTimesMatrix operation to SPIRV Dialect and add NoSideEffect trait to Matrix ops.

Diff Detail

Event Timeline

Hazem created this revision.Jun 26 2020, 11:35 AM
antiagainst requested changes to this revision.Jun 29 2020, 6:00 PM
antiagainst added inline comments.

By construct a MatrixType's element type is VectorType. so this is gonna succeed so there is no need to wrap it in a if statement. Essentially I think writing something like the following might look nicer:

auto leftM = leftMatrix.getNumElements();
auto leftVector = leftMatrix.getElementType().cast<VectorType>();
auto leftN = leftVector.getNumElements();
auto leftElemType = leftVector.getElementType();

// Similarly for rightMatrix and result Matrix

What do you think?

This revision now requires changes to proceed.Jun 29 2020, 6:00 PM
Hazem added inline comments.Jun 30 2020, 8:15 AM

I agree that this is better. In fact, I had some changes in mind for the matrix type, mainly:

  1. add getNumRows() and getNumColumns() methods
  2. refactor getNumElements to return total number of elements (rows*cols)
  3. refactor getElementType to return the type of a single cell (e.g, F32) instead of Vector
  4. Add getColumnType to return the Vector type of the matrix columns

These are minor changes coding wise, but they will make the verification methods much nicer than they are now, and the code easier to follow. And honestly they are intuitive for a matrix type. How that sounds?

antiagainst added inline comments.Jul 2 2020, 11:13 AM

Sorry for the delay. SGTM! Please go for it! :)

Hazem updated this revision to Diff 275265.Jul 2 2020, 5:00 PM
  • add helpers for the matrix type
  • update verification methods for matrix operations using the new helpers
Hazem marked 4 inline comments as done.Jul 3 2020, 10:24 AM
Hazem added inline comments.

Done, please have a look.

This revision is now accepted and ready to land.Jul 7 2020, 6:32 PM
This revision was automatically updated to reflect the committed changes.