Page MenuHomePhabricator

[MLIR][SPIRV] Adding new data type and decorators to SPIRV Dialect
ClosedPublic

Authored by Hazem on May 26 2020, 3:19 PM.

Details

Summary

Add a new data type to the SPIRV dialect: matrix type.
Add several decorators needed for successful deserialization of graphics shaders

Diff Detail

Event Timeline

Hazem created this revision.May 26 2020, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 3:19 PM
Hazem retitled this revision from [MLIR][SPIRV] Adding several decorators manually to support ser/des of recent version shaders. to [MLIR][SPIRV] Adding new data type and decorators to SPIRV Dialect.May 26 2020, 3:24 PM
Hazem edited the summary of this revision. (Show Details)
antiagainst requested changes to this revision.May 26 2020, 6:22 PM

Awesome, thanks a lot @Hazem for the contribution! I have a few nits inlined. One major thing: could you add (de)serialization round-trip tests for matrix under https://github.com/llvm/llvm-project/tree/master/mlir/test/Dialect/SPIRV/Serialization? The tests at the moment only covers IR parsing/printing round-tripping. :)

mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
65

Could you keep this alphabetically sorted?

77

Same here. Please keep it alphabetically sorted. :)

382

Maybe renaming the paramters as columnType and numColumns to be consistent with the spec? Similarly for the rest.

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
119–120

Same here for alphabetical order.

208

I think we can use CompositeType::isValid(t) to check the vector rank and num elements.

221

Generally error messages should start with lower case; this composes well in sentences.

408

Lower-case to start and no need for ending period.

mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp
165

Alphabetical order.

1027

Keep this together with the other field?

1043

No need for this getters given this is a struct anyway.

1080
if (auto vecType = elementType.dy_cast<VectorType>(...)) 
  if (vecType.getElementType().isa...)
This revision now requires changes to proceed.May 26 2020, 6:22 PM
Hazem marked 4 inline comments as done.May 27 2020, 8:05 AM
Hazem added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
208

CompositeType::isValid(t) will return true for IntegerTypes as well, Matrix need to have Float types, unless you meant to use it then re-check that the vector elements are Float and not Integer in the parseAndVerifyMatrixType. Please let me know which one you had in mind. Thanks.

Hazem marked 6 inline comments as done.May 27 2020, 9:21 AM
mravishankar requested changes to this revision.May 27 2020, 10:04 AM

Did a glance through of this. Mostly looks fine to me. Only question is the decorations added here dont seem to be related to the matrix type as such. If so can we split those into two separate CLs. And also I dont see any test for the roundtrip through SPIR-V binary for those decorations....

Did a glance through of this. Mostly looks fine to me. Only question is the decorations added here dont seem to be related to the matrix type as such. If so can we split those into two separate CLs. And also I dont see any test for the roundtrip through SPIR-V binary for those decorations....

Makes sense, and just to be sure I understood correctly, I will submit the decorators along with their tests in a separate patch and eliminate them from this matrix type patch?

Hazem updated this revision to Diff 266980.May 28 2020, 12:01 PM

Updating D80594: [MLIR][SPIRV] Addressed revision changes requested, and removed decorators to another CL as requested.

antiagainst accepted this revision.May 28 2020, 2:59 PM
antiagainst marked an inline comment as done.

Awesome, thanks for the contribution!

mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp
208

Actually both are fine. The current way makes sure we can have more meaningful error message, at the expense of duplicating the logic a bit. It's not too much duplication so it's fine. Thanks! :)

Do you have commit access to llvm-project mono repo? Let me know if not then I can land this for you.

Hazem added a comment.May 28 2020, 5:17 PM

Do you have commit access to llvm-project mono repo? Let me know if not then I can land this for you.

Based on the documentation here https://mlir.llvm.org/getting_started/Contributing/#first-time-contributors
and since I am a new contributor, I don't have direct access to commit and I have to request from you if you please to land this for me.
Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jun 2 2020, 1:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 1:46 PM

@Hazem: sure no problem! sorry for the delay; was OOO for a few days. :)