[PATCH] D80594: [MLIR][SPIRV] Adding new data type and decorators to SPIRV Dialect

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.

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



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h:64
 struct StructTypeStorage;
+struct MatrixTypeStorage;
 } // namespace detail
----------------
Could you keep this alphabetically sorted?


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h:75
   Struct,
-  LAST_SPIRV_TYPE = Struct,
+  Matrix,
+  LAST_SPIRV_TYPE = Matrix,
----------------
Same here. Please keep it alphabetically sorted. :) 


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h:381
+
+  static MatrixType get(Type elementType, uint32_t numCols);
+
----------------
Maybe renaming the paramters as `columnType` and `numColumns` to be consistent with the spec? Similarly for the rest.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:120
   addTypes<ArrayType, CooperativeMatrixNVType, ImageType, PointerType,
-           RuntimeArrayType, StructType>();
+           RuntimeArrayType, StructType, MatrixType>();
 
----------------
Same here for alphabetical order.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:208
+  if (auto t = type.dyn_cast<VectorType>()) {
+    if (t.getRank() != 1) {
+      parser.emitError(typeLoc, "only 1-D vector allowed but found ") << t;
----------------
I think we can use `CompositeType::isValid(t)` to check the vector rank and num elements.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:221
+    if (!t.getElementType().isa<FloatType>()) {
+      parser.emitError(typeLoc, "Matrix columns' elements must be of "
+                                "Float type, got ")
----------------
Generally error messages should start with lower case; this composes well in sentences. 


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:408
+  if (numCols < 2 || numCols > 4) {
+    parser.emitError(countLoc, "Matrix is expected to have 2, 3, or 4 "
+                               "columns.");
----------------
Lower-case to start and no need for ending period. 


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:164
   case TypeKind::Struct:
+  case TypeKind::Matrix:
     return true;
----------------
Alphabetical order. 


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:1027
+
+  Type elementType;
+
----------------
Keep this together with the other field?


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:1043
+
+  uint32_t getNumColumns() const { return numCols; }
+
----------------
No need for this getters given this is a struct anyway.


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp:1080
+bool MatrixType::isValidElementType(Type elementType) {
+  if (elementType.isa<VectorType>()) {
+    Type vectorElementsType =
----------------
```
if (auto vecType = elementType.dy_cast<VectorType>(...)) 
  if (vecType.getElementType().isa...)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80594