[PATCH] D80181: [mlir][spirv] Add remaining cooperative matrix instructions.

[Lei Zhang via Phabricator via llvm-commits]

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

Nice! Thanks Thomas!



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td:3006
 def SPV_Bool : TypeAlias<I1, "bool">;
+def SPV_Int32 : TypeAlias<I32, "Int32">;
 def SPV_Integer : AnyIntOfWidths<[8, 16, 32, 64]>;
----------------
Nit: can this be ordered after SPV_Integer?


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:56
+  );
+  let verifier = [{ return success(); }];
+}
----------------
I think we can add 

```
let assemblyFormat = "attr-dict `:` type($result)";
```

So that we don't need to manually write the parser and printer?


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:133
 
   let verifier = [{ return success(); }];
 }
----------------
I think we need verification on `$pointer`. At the moment every pointer will be accepted.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:175
+    cooperative-matrixmuladd-op ::= ssa-id `=` `spv.CooperativeMatrixMulAddNV`
+                              sssa-use `,` ssa-use `,` ssa-use ` : `
+                              cooperative-matrix-type
----------------
ssa-use


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:176
+                              sssa-use `,` ssa-use `,` ssa-use ` : `
+                              cooperative-matrix-type
+    ```
----------------
One general rule in MLIR regarding assembly is that it should be parsable on its own. I think we need to give at least two types (for `$a` and `$b`) for this? Otherwise by only looking at this op, I'm not able to tell what the type is for `$a` and `$b`.


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:203
+
+  let verifier = [{ return success(); }];
+}
----------------
I see we don't do any validation on these cooperative matrix ops. Do you plan to add them later?

Here we should at least additionally check that 1) `$a`, `$b`, `$c`, and `$result` has consistent dimensions: MxK, KxN, MxN, MxN, and 2) they are at the same scope. Basically going through the description and verify what's written there is checked. :) 


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:204
+  let verifier = [{ return success(); }];
+}
+
----------------
Similarly here I think we can do 

```
let assemblyFormat = "$a, $b, $c `:` type($a), type($b), type($c, $result)
```

(`type($c)` is kinda redundant given that we can deduce it but I don't care that much. Also it makes the mapping very clear.)


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVCooperativeMatrixOps.td:265
+
+  let verifier = [{ return success(); }];
+}
----------------
Similarly we need to verify the requirements on `$pointer`.


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

https://reviews.llvm.org/D80181