[mlir][Linalg] Fix Linalg EDSC builders

[nicolasvasilache]

Summary

This diff fixes the fact that the method mlir::edsc::makeGenericLinalgOp
incorrectly adds 2 blocks to Linalg ops.

Tests are updated accordingly.

[merge_guards_bot]

Unit tests: pass. 61804 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[asaadaldien]

LGTM! an inline question trying to understand the Builder blocks scoping here.

mlir/lib/Dialect/Linalg/EDSC/Builders.cpp

186–187

OpBuilder bb(region);

196

Does the builder create a new block instead of mutating region.begin that we have to clean up ?

[nicolasvasilache]

mlir/lib/Dialect/Linalg/EDSC/Builders.cpp

196

Yes, currently it does.
I can improve this by extending ScopedContext slightly I think.
This is because the mechanism predates regions and has not yet been updated.

[nicolasvasilache]

Address review comments.

[merge_guards_bot]

Unit tests: pass. 61912 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[asaadaldien]

mlir/lib/EDSC/Builders.cpp

307

typo s/BockBuilder/BlockBuilder

mlir/include/mlir/EDSC/Builders.h

public:
   /// Captures the new block in `bh` and its arguments into `args`.
   /// Enters the new mlir::Block* and sets the insertion point to its end.
   ///
   /// Prerequisites:
   ///   The ValueHandle `args` are typed delayed ValueHandles; i.e. they are
   ///   not yet bound to mlir::Value.
   BlockBuilder(BlockHandle *bh, ArrayRef<ValueHandle *> args);
 
+  /// Constructs a new mlir::Block with argument types derived from `args` and
+  /// appends it as the last block in the region.
+  /// Captures the new block in `bh` and its arguments into `args`.
+  /// Enters the new mlir::Block* and sets the insertion point to its end.
+  ///
+  /// Prerequisites:
+  ///   The ValueHandle `args` are typed delayed ValueHandles; i.e. they are
+  ///   not yet bound to mlir::Value.
+  BlockBuilder(BlockHandle *bh, Region &region, ArrayRef<ValueHandle *> args);
+
   /// The only purpose of this operator is to serve as a sequence point so that
   /// the evaluation of `fun` (which build IR snippets in a scoped fashion) is
   /// scoped within a BlockBuilder.
   void operator()(function_ref<void(void)> fun = nullptr);
 
 private:
   BlockBuilder(BlockBuilder &) = delete;
   BlockBuilder &operator=(BlockBuilder &other) = delete;
public:
   /// BlockHandle is a value type, use the default copy constructor and
   /// assignment operator.
   BlockHandle(const BlockHandle &) = default;
   BlockHandle &operator=(const BlockHandle &) = default;
 
   /// Delegates block creation to MLIR and wrap the resulting mlir::Block.
   static BlockHandle create(ArrayRef<Type> argTypes);
 
+  /// Delegates block creation to MLIR and wrap the resulting mlir::Block.
+  static BlockHandle createInRegion(Region &region, ArrayRef<Type> argTypes);
+
   operator bool() { return block != nullptr; }
   operator mlir::Block *() { return block; }
   mlir::Block *getBlock() { return block; }
 
 private:
   mlir::Block *block;
 };
 

mlir/lib/Dialect/Linalg/EDSC/Builders.cpp

Operation *mlir::edsc::makeGenericLinalgOp(
 
   using namespace edsc;
   SmallVector<Type, 4> blockTypes;
   blockTypes.reserve(values.size());
   for (auto it : llvm::enumerate(values))
     blockTypes.push_back((it.index() < nViews)
                              ? getElementTypeOrSelf(it.value())
                              : it.value().getType());
 
-  assert(op->getRegions().front().empty());
-  op->getRegions().front().push_front(new Block);
-  OpBuilder bb(op->getRegions().front());
-  ScopedContext scope(bb, op->getLoc());
+  assert(op->getNumRegions() == 1);
+  assert(op->getRegion(0).empty());
+  OpBuilder opBuilder(op);
+  ScopedContext scope(opBuilder, op->getLoc());
   BlockHandle b;
   auto handles = makeValueHandles(blockTypes);
-  BlockBuilder(&b, makeHandlePointers(MutableArrayRef<ValueHandle>(handles)))(
+  BlockBuilder(&b, op->getRegion(0),
+               makeHandlePointers(MutableArrayRef<ValueHandle>(handles)))(
       [&] { regionBuilder(b.getBlock()->getArguments()); });
+  assert(op->getRegion(0).getBlocks().size() == 1);
   return op;
 }
 
 void mlir::edsc::ops::macRegionBuilder(ArrayRef<BlockArgument> args) {
   using edsc::op::operator+;
   using edsc::op::operator*;
   assert(args.size() == 3 && "expected 3 block arguments");
   ValueHandle a(args[0]), b(args[1]), c(args[2]);

mlir/lib/EDSC/Builders.cpp

BlockHandle mlir::edsc::BlockHandle::create(ArrayRef<Type> argTypes) {
   // We do not want this behavior when using declarative builders with nesting.
   currentB.setInsertionPoint(ib, ip);
   for (auto t : argTypes) {
     res.block->addArgument(t);
   }
   return res;
 }
 
+BlockHandle mlir::edsc::BlockHandle::createInRegion(Region &region,
+                                                    ArrayRef<Type> argTypes) {
+  auto &currentB = ScopedContext::getBuilder();
+  BlockHandle res;
+  region.push_back(new Block);
+  res.block = &region.back();
+  // createBlock sets the insertion point inside the block.
+  // We do not want this behavior when using declarative builders with nesting.
+  OpBuilder::InsertionGuard g(currentB);
+  currentB.setInsertionPoint(res.block, res.block->begin());
+  for (auto t : argTypes) {
+    res.block->addArgument(t);
+  }
+  return res;
+}
+
 static Optional<ValueHandle> emitStaticFor(ArrayRef<ValueHandle> lbs,
                                            ArrayRef<ValueHandle> ubs,
                                            int64_t step) {
   if (lbs.size() != 1 || ubs.size() != 1)
     return Optional<ValueHandle>();
 
   auto *lbDef = lbs.front().getValue().getDefiningOp();
   auto *ubDef = ubs.front().getValue().getDefiningOp();
mlir::edsc::BlockBuilder::BlockBuilder(BlockHandle *bh,
   }
   *bh = BlockHandle::create(types);
   for (auto it : llvm::zip(args, bh->getBlock()->getArguments())) {
     *(std::get<0>(it)) = ValueHandle(std::get<1>(it));
   }
   enter(bh->getBlock());
 }
 
+mlir::edsc::BlockBuilder::BlockBuilder(BlockHandle *bh, Region &region,
+                                       ArrayRef<ValueHandle *> args) {
+  assert(!*bh && "BlockHandle already captures a block, use "
+                 "the explicit BockBuilder(bh, Append())({}) syntax instead.");
+  SmallVector<Type, 8> types;
+  for (auto *a : args) {
+    assert(!a->hasValue() &&
+           "Expected delayed ValueHandle that has not yet captured.");
+    types.push_back(a->getType());
+  }
+  *bh = BlockHandle::createInRegion(region, types);
+  for (auto it : llvm::zip(args, bh->getBlock()->getArguments())) {
+    *(std::get<0>(it)) = ValueHandle(std::get<1>(it));
+  }
+  enter(bh->getBlock());
+}
+
 /// Only serves as an ordering point between entering nested block and creating
 /// stmts.
 void mlir::edsc::BlockBuilder::operator()(function_ref<void(void)> fun) {
   // Call to `exit` must be explicit and asymmetric (cannot happen in the
   // destructor) because of ordering wrt comma operator.
   if (fun)
     fun();
   exit();

mlir/test/EDSC/builder-api-test.cpp

TEST_FUNC(linalg_pointwise_test) {
   f.erase();
 }
 
 // clang-format off
 // CHECK-LABEL: func @linalg_matmul
 //       CHECK:   linalg.generic {args_in = 2 : i64, args_out = 1 : i64,
 // CHECK-SAME: indexing_maps = [affine_map<(d0, d1, d2) -> (d0, d2)>, affine_map<(d0, d1, d2) -> (d2, d1)>, affine_map<(d0, d1, d2) -> (d0, d1)>],
 // CHECK-SAME: iterator_types = ["parallel", "parallel", "reduction"]}
-///      CHECK:   ^bb1(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
+///      CHECK:   ^bb0(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
 //       CHECK:     %[[a3:.*]] = mulf %[[a0]], %[[a1]] : f32
 //       CHECK:     %[[a4:.*]] = addf %[[a2]], %[[a3]] : f32
 //       CHECK:     linalg.yield %[[a4]] : f32
 //       CHECK:   }: memref<?x?xf32>, memref<?x?xf32>, memref<?x?xf32>
 // clang-format on
 TEST_FUNC(linalg_matmul_test) {
   using namespace edsc;
   using namespace edsc::ops;

 
 // clang-format off
 // CHECK-LABEL: func @linalg_conv_nhwc
 //       CHECK:   linalg.generic {args_in = 2 : i64, args_out = 1 : i64,
 // CHECK-SAME: indexing_maps = [affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d0, d2 * 3 + d4 * 5, d3 * 4 + d5 * 6, d6)>,
 // CHECK-SAME: affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d4, d5, d6, d1)>,
 // CHECK-SAME: affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d0, d2, d3, d1)>],
 // CHECK-SAME: iterator_types = ["parallel", "parallel", "parallel", "parallel", "reduction", "reduction", "reduction"]}
-///      CHECK:   ^bb1(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
+///      CHECK:   ^bb0(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
 //       CHECK:     %[[a3:.*]] = mulf %[[a0]], %[[a1]] : f32
 //       CHECK:     %[[a4:.*]] = addf %[[a2]], %[[a3]] : f32
 //       CHECK:     linalg.yield %[[a4]] : f32
 //       CHECK:   }: memref<?x?x?x?xf32>, memref<?x?x?x?xf32>, memref<?x?x?x?xf32>
 // clang-format on
 TEST_FUNC(linalg_conv_nhwc) {
   using namespace edsc;
   using namespace edsc::ops;

 
 // clang-format off
 // CHECK-LABEL: func @linalg_dilated_conv_nhwc
 //       CHECK:   linalg.generic {args_in = 2 : i64, args_out = 1 : i64,
 // CHECK-SAME: indexing_maps = [affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d0, d3 * 3 + d5 * 5, d4 * 4 + d6 * 6, d2)>,
 // CHECK-SAME: affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d5, d6, d2, d1)>,
 // CHECK-SAME: affine_map<(d0, d1, d2, d3, d4, d5, d6) -> (d0, d3, d4, d1 + d2 * 7)>],
 // CHECK-SAME: iterator_types = ["parallel", "parallel", "parallel", "parallel", "parallel", "reduction", "reduction"]}
-//       CHECK:   ^bb1(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
+//       CHECK:   ^bb0(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
 //       CHECK:     %[[a3:.*]] = mulf %[[a0]], %[[a1]] : f32
 //       CHECK:     %[[a4:.*]] = addf %[[a2]], %[[a3]] : f32
 //       CHECK:     linalg.yield %[[a4]] : f32
 //       CHECK:   }: memref<?x?x?x?xf32>, memref<?x?x?x?xf32>, memref<?x?x?x?xf32>
 // clang-format on
 TEST_FUNC(linalg_dilated_conv_nhwc) {
   using namespace edsc;
   using namespace edsc::ops;