[PATCH] D75653: [mlir] Introduce an intrinsic for llvm.matrix.multiply

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:771
+
+// TODO(ntv, zinenko): Provide more idiomatic tblgen support.
+// OTOH there are 4 intrinsics total atm so..
----------------
Can you elaborate?


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:797
+      Arguments<(
+        ins LLVM_Type:$A, LLVM_Type:$B, I32Attr:$M, I32Attr:$N, I32Attr:$K)> {
+  string llvmBuilder = [{
----------------
Could have at least have a textual description to understand what ABMNK mean? These will be the names of accessor functions as well...


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:797
+      Arguments<(
+        ins LLVM_Type:$A, LLVM_Type:$B, I32Attr:$M, I32Attr:$N, I32Attr:$K)> {
+  string llvmBuilder = [{
----------------
ftynse wrote:
> Could have at least have a textual description to understand what ABMNK mean? These will be the names of accessor functions as well...
Also, any reason to chose I32 attribute?


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:815
+        llvm::ConstantInt::get(builder.getInt32Ty(), $M.getZExtValue()),
+        // LLVM call is M, K, N .. go figure
+        llvm::ConstantInt::get(builder.getInt32Ty(), $K.getZExtValue()),
----------------
I suppose the Op arguments are also in that order, so it would make sense to reorder them in Arguments<>.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:819
+      }
+    );
+  }];
----------------
This looks sufficiently long to go to .cpp instead.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:824
+              "Value A, Value B, IntegerAttr M, IntegerAttr N, IntegerAttr K",
+    [{
+      result.addAttribute("M", M);
----------------
This builder seems to ignore operands. I would also expect such a straightforward builder to be autogenerated.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:832
+  let assemblyFormat =
+    "$A `,` $B attr-dict `:` type($A) `,` type($B) `->` type($res)";
+}
----------------
Could we use a more conventional function-type notation: `'(' type($A) ',' type($B) ') -> ' type($res)`  (or maybe there's support for that directly in the formatter)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75653