This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Enable composite instructions for cooperative matrix type.
ClosedPublic

Authored by ThomasRaoux on May 20 2020, 2:00 PM.

Details

Summary

Enable inset/extract/construct composite ops as well as access chain for cooperative matrix. ConstantComposite requires more change and will be done in a separate patch.
Also fix the getNumElements function for coopMatrix per feedback from Jeff Bolz. The number of element is implementation dependent so it cannot be known at compile time.

Diff Detail

Event Timeline

ThomasRaoux created this revision.May 20 2020, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2020, 2:00 PM
antiagainst requested changes to this revision.May 20 2020, 3:07 PM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
143

Could you add a comment to getNumElements to mention that this method being false is a pre-condition? (With that actually it would be nice to rename this as hasCompileTimeKnownNumElements or something so that we don't always use it as the negate form. But a minor issue.)

mlir/test/Dialect/SPIRV/Serialization/cooperative-matrix.mlir
100

Can we return %1 here so we can check the return type too? Right now it's hidden.

mlir/test/Dialect/SPIRV/composite-ops.mlir
25

I think this should only take one operand.

This revision now requires changes to proceed.May 20 2020, 3:07 PM
ThomasRaoux marked 3 inline comments as done.
ThomasRaoux added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
143

Added a comment about getNumElements and change the function name.

mlir/test/Dialect/SPIRV/composite-ops.mlir
25

Thanks for catching that, changed it and added a negative test.

antiagainst accepted this revision.May 21 2020, 5:07 AM

Nice, thanks! Just one more nit.

mlir/test/Dialect/SPIRV/Serialization/cooperative-matrix.mlir
96

Note that according to the spec: "Cooperative matrix types (or types containing them) can only be allocated in Function or Private storage classes." So here we cannot have a pointer pointing to a coop matrix in a StorageBuffer. We should use Function/Private here. Although this is test; still would be nice to be valid. So later when we enforce this rule these tests can still pass as-is. :)

mlir/test/Dialect/SPIRV/cooperative-matrix.mlir
98

Same here.

This revision is now accepted and ready to land.May 21 2020, 5:07 AM
ThomasRaoux marked an inline comment as done.
ThomasRaoux added inline comments.
mlir/test/Dialect/SPIRV/Serialization/cooperative-matrix.mlir
96

Good point, I changed it.

This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.May 27 2020, 1:59 PM
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
1138

nit: Merge this if into the else.