Skip to content

Commit cb6e196

Browse files
authored
[MLIR] Forward generated OpTy::create arguments (llvm#170012)
The recent changes in the MLIR TableGen interface for generated OpTy::build functions involves a new OpTy::create function that is generated passing arguments without forwarding. This is problematic with arguments that are move only such as `std::unique_ptr`. My particular use case involves `std::unique_ptr<mlir::Region>` which is desirable as the `mlir::OperationState` object accepts calls to `addRegion(std::unique_ptr<mlir::Region>`. In Discord, the use of `extraClassDeclarations` was suggested which I may go with regardless since I still have to define the builder function anyways, but perhaps you would consider this trivial change as it supports a broader class of argument types for this approach. Consider the declaration in TableGen: ``` let builders = [ OpBuilder<(ins "::mlir::Value":$cdr, "::mlir::ValueRange":$packs, "std::unique_ptr<::mlir::Region>":$body)> ]; ``` Which currently generates: ```cpp ExpandPacksOp ExpandPacksOp::create(::mlir::OpBuilder &builder, ::mlir::Location location, ::mlir::Value cdr, ::mlir::ValueRange packs, std::unique_ptr<::mlir::Region> body) { ::mlir::OperationState __state__(location, getOperationName()); build(builder, __state__, std::forward<decltype(cdr)>(cdr), std::forward<decltype(packs)>(packs), std::forward<decltype(body)>(body)); auto __res__ = ::llvm::dyn_cast<ExpandPacksOp>(builder.create(__state__)); assert(__res__ && "builder didn't return the right type"); return __res__; } ``` With this change it will generate: ```cpp ExpandPacksOp ExpandPacksOp::create(::mlir::OpBuilder &builder, ::mlir::Location location, ::mlir::Value cdr, ::mlir::ValueRange packs, std::unique_ptr<::mlir::Region>&&body) { ::mlir::OperationState __state__(location, getOperationName()); build(builder, __state__, static_cast<decltype(cdr)>(cdr), std::forward<decltype(packs)>(packs), std::forward<decltype(body)>(body)); auto __res__ = ::llvm::dyn_cast<ExpandPacksOp>(builder.create(__state__)); assert(__res__ && "builder didn't return the right type"); return __res__; } ``` Another option could be to make this function a template but then it would not be hidden in the generated translation unit. I don't know if that was the original intent. Thank you for your consideration.
1 parent f3501d7 commit cb6e196

File tree

2 files changed

+10
-3
lines changed

2 files changed

+10
-3
lines changed

mlir/test/mlir-tblgen/op-decl-and-defs.td

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,14 +235,14 @@ def NS_FOp : NS_Op<"op_with_all_types_constraint",
235235

236236
// DEFS: FOp FOp::create(::mlir::OpBuilder &builder, ::mlir::Location location, ::mlir::Value a) {
237237
// DEFS: ::mlir::OperationState __state__(location, getOperationName());
238-
// DEFS: build(builder, __state__, a);
238+
// DEFS: build(builder, __state__, std::forward<decltype(a)>(a));
239239
// DEFS: auto __res__ = ::llvm::dyn_cast<FOp>(builder.create(__state__));
240240
// DEFS: assert(__res__ && "builder didn't return the right type");
241241
// DEFS: return __res__;
242242
// DEFS: }
243243

244244
// DEFS: FOp FOp::create(::mlir::ImplicitLocOpBuilder &builder, ::mlir::Value a) {
245-
// DEFS: return create(builder, builder.getLoc(), a);
245+
// DEFS: return create(builder, builder.getLoc(), std::forward<decltype(a)>(a));
246246
// DEFS: }
247247

248248
def NS_GOp : NS_Op<"op_with_fixed_return_type", []> {

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2641,7 +2641,14 @@ void OpEmitter::genInlineCreateBody(
26412641
std::string nonBuilderStateArgs = "";
26422642
if (!nonBuilderStateArgsList.empty()) {
26432643
llvm::raw_string_ostream nonBuilderStateArgsOS(nonBuilderStateArgs);
2644-
interleaveComma(nonBuilderStateArgsList, nonBuilderStateArgsOS);
2644+
interleave(
2645+
nonBuilderStateArgsList,
2646+
[&](StringRef name) {
2647+
nonBuilderStateArgsOS << "std::forward<decltype(" << name << ")>("
2648+
<< name << ')';
2649+
},
2650+
[&] { nonBuilderStateArgsOS << ", "; });
2651+
26452652
nonBuilderStateArgs = ", " + nonBuilderStateArgs;
26462653
}
26472654
if (cWithLoc)

0 commit comments

Comments
 (0)