[PATCH] D78327: [mlir][Linalg] Create a named batchmatmul op and pipe it through.

[Uday Bondhugula via Phabricator via llvm-commits]

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

Mostly superficial comments...



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOpsInterface.td:94
+    InterfaceMethod<[{
+        Return the `i`-th output shaped type, irrespective of buffer of tensor
+        type.
----------------
of -> or


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:764
 
-template <typename GenericOpType>
-static LogicalResult verifyYield(YieldOp op, GenericOpType genericOp) {
+static LogicalResult verifyYield(YieldOp op, LinalgOp linalgOpInterface) {
   // The operand number and types must match the view element types.
----------------
Doc comments please. 


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:790-793
+  if (auto linalgOp = dyn_cast<LinalgOp>(parentOp))
+    return verifyYield(op, cast<LinalgOp>(parentOp));
 
+  return op.emitOpError("expected parent op with LinalgOp interface");
----------------
All of this looks problematic. Do you need this on YieldOp's verifier instead of LinalgOp's verifier? (Check for its terminator there and verify?)


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:1085
+                                  TypeRange tensorResultTypes) {
+  Region &bodyRegion = *result.addRegion();
+  Block *body = new Block();
----------------
Nit: `bodyRegion` is weird. Just `region` is good or `opRegion`. 


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:1096
+  OpBuilder opBuilder(builder.getContext());
+  opBuilder.setInsertionPoint(&bodyRegion.front(), bodyRegion.front().begin());
+  mlir::edsc::ScopedContext scope(opBuilder, builder.getUnknownLoc());
----------------
`setInsertionPointToStart(&bodyRegion.front())`


================
Comment at: mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:1103
+static void printNamedStructuredOp(OpAsmPrinter &p, NamedStructuredOpType op) {
+  p << op.getOperationName() << " ";
+  p.printOptionalAttrDict(op.getAttrs());
----------------
Micronit: " " -> ' '; likewise below. 


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:124
 namespace {
+
 template <typename IndexedValueType, typename LinalgOpType>
----------------
Doc comment please. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78327