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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 5 inline comments as done.
nicolasvasilache added a comment.

> I now see the relevant method. But to use the builder, you only need a block or a region, and not an op(?). You could do an addRegion(), followed by pushing a new block into it, and then create the builder to insert. Am I missing something?

This is what I started with but along the line I realized that `region.getContext()` requires `region` to be attached to a `Operation*`.
See: https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/Region.h#L31

However this was a red herring only due to the fact that I was constructing `OpBuilder` from `region`.
I.e. this fails:

  Region *region = result.addRegion();
  OpBuilder opBuilder(region);

This works fine:

  Region *region = result.addRegion();
  OpBuilder opBuilder(context);
  opBuilder.setInsertionPoint(&region.front(), region.front().begin());

Thanks for noting!



================
Comment at: mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h:123
   }
-  /// Return the `i`-th input buffer type.
+  /// Return the `i`-th input shaped type, irrespective of buffer of tensor
+  /// type.
----------------
bondhugula wrote:
> of -> or
> buffer -> memref
First typo is corrected.
For the second, Linalg uses the terminology tensors and buffers consistently.
Memref is a particular implementation of buffers, the only one supported for now.


================
Comment at: mlir/test/Dialect/Linalg/roundtrip.mlir:628
+                %ta3: tensor<?x?x?xf32>, %tb3: tensor<?x?xf32>, %tc3: tensor<?x?x?xf32>) {
+  linalg.batchmatmul(%a3, %b3, %c3): (memref<?x?x?xf32>, memref<?x?xf32>, memref<?x?x?xf32>) -> ()
+  linalg.batchmatmul(%ta3, %tb3, %c3): (tensor<?x?x?xf32>, tensor<?x?xf32>, memref<?x?x?xf32>) -> ()
----------------
bondhugula wrote:
> For the custom format, better to drop the parentheses
> `linalg.batchmatmul %a3, %b3, %c3 : `
> 
> FWIW, it'd be better with an underscore batch_matmul.
Fair enough.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78327