[MLIR] Replace OpBuilder(Block) with atBlockBegin and atBlockEnd

[tpopp]

Summary

OpBuilder(Block) is specifically replaced with OpBuilder::atBlockEnd(Block);

This is to make insertion behavior clear due to there being no one
correct answer for which location in a block the default insertion
point should be.

[ftynse]

Could we rather have static constructors that make it explicit what the insertion point is?

[tpopp]

In D77060#1950238, @ftynse wrote:

Could we rather have static constructors that make it explicit what the insertion point is?

Static builders (assuming you mean OpBuilder b = OpBuilder::Get(body, body->begin()) ) and an explicit insertion point are orthogonal right? Or do you mean OpBuilder::AtBlockBegin(body)?

[ftynse]

Indeed, I mean adding OpBuilder::atBlockBeginand OpBuilder::atBlockEnd and removing the default constructor. Deferring to @rriddle though :)

[mehdi_amini]

I prefer the explicit version as well.
In the past I tried to change OpBuilder(Operation *) in a similar way by forcing an enum: OpBuilder builder(op, OpBuilder::After) / OpBuilder builder(op, OpBuilder::Before), but a factory method is good as well.

[rriddle]

I'm +1 on the explicit factory methods

[tpopp]

Create AtBlockBegin and AtBlockEnd. Remove OpBuilder(Block).

[ftynse]

mlir/include/mlir/IR/Builders.h

198

Nit: MLIR's convention for function names is to start with a small letter.

[tpopp]

Follow naming convention for atBlockBegin and atBlockEnd.

[rriddle]

mlir/include/mlir/IR/Builders.h

196

This description doesn't really describe this function.

mlir/include/mlir/IR/Builders.h

public:
   virtual ~OpBuilder();
 
   /// Create a builder and set insertion point to the given operation, which
   /// will cause subsequent insertions to go right before it.
   explicit OpBuilder(Operation *op) : Builder(op->getContext()) {
     setInsertionPoint(op);
   }
 
-  explicit OpBuilder(Block *block) : OpBuilder(block, block->end()) {}
-
   OpBuilder(Block *block, Block::iterator insertPoint)
       : OpBuilder(block->getParent()) {
     setInsertionPoint(block, insertPoint);
   }
 
+  /// Create a builder and set the insertion point to the specified location in
+  /// the Block.
+  static OpBuilder AtBlockBegin(Block *block) {
+    return OpBuilder(block, block->begin());
+  }
+
+  static OpBuilder AtBlockEnd(Block *block) {
+    return OpBuilder(block, block->end());
+  }
+
   /// This class represents a saved insertion point.
   class InsertPoint {
   public:
     /// Creates a new insertion point which doesn't point to anything.
     InsertPoint() = default;
 
     /// Creates a new insertion point at the given location.
     InsertPoint(Block *insertBlock, Block::iterator insertPt)

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

Block *spirv::LoopOp::getMergeBlock() {
   return &body().back();
 }
 
 void spirv::LoopOp::addEntryAndMergeBlock() {
   assert(body().empty() && "entry and merge block already exist");
   body().push_back(new Block());
   auto *mergeBlock = new Block();
   body().push_back(mergeBlock);
-  OpBuilder builder(mergeBlock);
+  OpBuilder builder = OpBuilder::AtBlockEnd(mergeBlock);
 
   // Add a spv._merge op into the merge block.
   builder.create<spirv::MergeOp>(getLoc());
 }
 
 //===----------------------------------------------------------------------===//
 // spv._merge
 //===----------------------------------------------------------------------===//
Block *spirv::SelectionOp::getMergeBlock() {
   // The last block is the loop merge block.
   return &body().back();
 }
 
 void spirv::SelectionOp::addMergeBlock() {
   assert(body().empty() && "entry and merge block already exist");
   auto *mergeBlock = new Block();
   body().push_back(mergeBlock);
-  OpBuilder builder(mergeBlock);
+  OpBuilder builder = OpBuilder::AtBlockEnd(mergeBlock);
 
   // Add a spv._merge op into the merge block.
   builder.create<spirv::MergeOp>(getLoc());
 }
 
 spirv::SelectionOp spirv::SelectionOp::createIfThen(
     Location loc, Value condition,
     function_ref<void(OpBuilder *builder)> thenBody, OpBuilder *builder) {