[mlir] Make OpBuilder::createOperation to accept raw inputs

[Chia-hungDuan]

Summary

This provides a way to create an operation without manipulating
OperationState directly. This is useful for creating unregistered ops.

[jpienaar]

mlir/test/lib/Dialect/Test/TestDialect.cpp

798

Is SmallVector / temporary needed or is this for readability? (e.g., could we use {lhs, rhs} below instead of innerOperands and let ValueRange's constructor take care of it).

[Chia-hungDuan]

mlir/test/lib/Dialect/Test/TestDialect.cpp

798

Yes, it just for readability. Or i would use /*operands=*/{lhs, rhs}, either way should be fine.

[rriddle]

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

mlir/test/lib/Dialect/Test/TestDialect.cpp

798

I would rather inline it. As an aside, if we want to temp variable, we should use an array instead of SmallVector.

[Chia-hungDuan]

In D120787#3355227, @rriddle wrote:

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

[rriddle]

In D120787#3356135, @Chia-hungDuan wrote:

In D120787#3355227, @rriddle wrote:

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

OperationState isn't really just an implementation detail though, given that it is heavily exposed (e.g. all builders use it, parsers use it, etc.).

[Chia-hungDuan]

In D120787#3356136, @rriddle wrote:

In D120787#3356135, @Chia-hungDuan wrote:

In D120787#3355227, @rriddle wrote:

A part of me wants to ask what the trade off of this is vs just doing createOperation(OperationState(...))? Well we could also rename createOperation to create(to match the other API), which would make create(OperationState(...)) less redundant to read.

Either way, if we think this is a common enough use case I would be okay with adding the API for it.

OperationState is more like an implementation detail thing and having this makes the preparation of creating operation the same in between registered and unregistered op. I mean, when a user wants to create an unregistered op, they don't need to know there's a OperationState structure to work with. They could have same experience of creating operation. So far, I'm not sure if there's anything bad if we provide this.

I think we can make the createOperation(OperationState) private and having create(...) as public. Or do you think it's better to have both of them for users? I.e., have both create(OperationState) and create(...) in public?

OperationState isn't really just an implementation detail though, given that it is heavily exposed (e.g. all builders use it, parsers use it, etc.).

Sorry, I didn't say it clearly. For user's perspective, writing pass or patterns, they don't need to know OperationState if they only work on registered op.

[mehdi_amini]

LGTM: it is nicer to write than the having to explicitly wrap into an OperationState.

But please remove the SmallVector!
(and wait for River's approval as well)

mlir/include/mlir/IR/Builders.h

412

I would take a StringAttr instead of a StringRef: it encourages a more efficient API

[Chia-hungDuan]

Use StringAttr instead and updated some cases to use the create without OperationState

[Chia-hungDuan]

Also changed the createOperation to create.

After checking some cases, OperationState is useful when building some complicated cases. For example, they don't need to allocate individual containers for each argument. So I think having both of them will be better.

[Chia-hungDuan]

@rriddle, a gentle ping!

[Chia-hungDuan]

Rebase

mlir/include/mlir/IR/Builders.h

public:
   //===--------------------------------------------------------------------===//
 
   /// Insert the given operation at the current insertion point and return it.
   Operation *insert(Operation *op);
 
   /// Creates an operation given the fields represented as an OperationState.
   Operation *createOperation(const OperationState &state);
 
+  /// Creates an operation with the given fields.
+  Operation *
+  createOperation(Location loc, StringRef opName, ValueRange operands,
+                  TypeRange types, ArrayRef<NamedAttribute> attributes = {},
+                  BlockRange successors = {},
+                  MutableArrayRef<std::unique_ptr<Region>> regions = {});
+
 private:
   /// Helper for sanity checking preconditions for create* methods below.
   template <typename OpT>
   RegisteredOperationName getCheckRegisteredInfo(MLIRContext *ctx) {
     Optional<RegisteredOperationName> opName =
         RegisteredOperationName::lookup(OpT::getOperationName(), ctx);
     if (LLVM_UNLIKELY(!opName)) {
       llvm::report_fatal_error(

mlir/lib/IR/Builders.cpp

return createBlock(insertBefore->getParent(), Region::iterator(insertBefore),
                      argTypes, locs);
 }
 
 /// Create an operation given the fields represented as an OperationState.
 Operation *OpBuilder::createOperation(const OperationState &state) {
   return insert(Operation::create(state));
 }
 
+/// Creates an operation with the given fields.
+Operation *
+OpBuilder::createOperation(Location loc, StringRef opName, ValueRange operands,
+                           TypeRange types, ArrayRef<NamedAttribute> attributes,
+                           BlockRange successors,
+                           MutableArrayRef<std::unique_ptr<Region>> regions) {
+  OperationState state(loc, opName, operands, types, attributes, successors,
+                       regions);
+  return createOperation(state);
+}
+
 /// Attempts to fold the given operation and places new results within
 /// 'results'. Returns success if the operation was folded, failure otherwise.
 /// Note: This function does not erase the operation on a successful fold.
 LogicalResult OpBuilder::tryFold(Operation *op,
                                  SmallVectorImpl<Value> &results) {
   ResultRange opResults = op->getResults();
 
   results.reserve(opResults.size());

mlir/test/lib/Dialect/Test/TestDialect.cpp

ParseResult PrettyPrintedRegionOp::parse(OpAsmParser &parser,
   // result and operand types all same as the test-op's first operand.
   Type innerOpType = opFntype.getInput(0);
   Value lhs = block.addArgument(innerOpType, opLoc);
   Value rhs = block.addArgument(innerOpType, opLoc);
 
   OpBuilder builder(parser.getBuilder().getContext());
   builder.setInsertionPointToStart(&block);
 
-  OperationState innerOpState(opLoc, innerOpName);
-  innerOpState.operands.push_back(lhs);
-  innerOpState.operands.push_back(rhs);
-  innerOpState.addTypes(innerOpType);
-
-  Operation *innerOp = builder.createOperation(innerOpState);
+  SmallVector<Value, 2> innerOperands{lhs, rhs};
+  Operation *innerOp =
+      builder.createOperation(opLoc, innerOpName, innerOperands, innerOpType);
 
   // Insert a return statement in the block returning the inner-op's result.
   builder.create<TestReturnOp>(innerOp->getLoc(), innerOp->getResults());
 
   // Populate the op operation-state with result-type and location.
   result.addTypes(opFntype.getResults());
   result.location = innerOp->getLoc();