[Mlir-commits] [mlir] cb6e196 - [MLIR] Forward generated OpTy::create arguments (#170012)

[llvmlistbot at llvm.org]

Author: Jason Rice
Date: 2025-12-01T21:27:23-08:00
New Revision: cb6e1967c4f5239170b7c088ba6f86910ae66a63

URL: https://github.com/llvm/llvm-project/commit/cb6e1967c4f5239170b7c088ba6f86910ae66a63
DIFF: https://github.com/llvm/llvm-project/commit/cb6e1967c4f5239170b7c088ba6f86910ae66a63.diff

LOG: [MLIR] Forward generated OpTy::create arguments (#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.

Added: 
    

Modified: 
    mlir/test/mlir-tblgen/op-decl-and-defs.td
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/test/mlir-tblgen/op-decl-and-defs.td b/mlir/test/mlir-tblgen/op-decl-and-defs.td
index 0e87373b2f6c2..80dedb8475b9e 100644
--- a/mlir/test/mlir-tblgen/op-decl-and-defs.td
+++ b/mlir/test/mlir-tblgen/op-decl-and-defs.td
@@ -235,14 +235,14 @@ def NS_FOp : NS_Op<"op_with_all_types_constraint",
 
 // DEFS: FOp FOp::create(::mlir::OpBuilder &builder, ::mlir::Location location, ::mlir::Value a) {
 // DEFS:   ::mlir::OperationState __state__(location, getOperationName());
-// DEFS:   build(builder, __state__, a);
+// DEFS:   build(builder, __state__, std::forward<decltype(a)>(a));
 // DEFS:   auto __res__ = ::llvm::dyn_cast<FOp>(builder.create(__state__));
 // DEFS:   assert(__res__ && "builder didn't return the right type");
 // DEFS:   return __res__;
 // DEFS: }
 
 // DEFS: FOp FOp::create(::mlir::ImplicitLocOpBuilder &builder, ::mlir::Value a) {
-// DEFS:   return create(builder, builder.getLoc(), a);
+// DEFS:   return create(builder, builder.getLoc(), std::forward<decltype(a)>(a));
 // DEFS: }
 
 def NS_GOp : NS_Op<"op_with_fixed_return_type", []> {

diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 3b10842f2a127..dbae5d9258d04 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -2641,7 +2641,14 @@ void OpEmitter::genInlineCreateBody(
   std::string nonBuilderStateArgs = "";
   if (!nonBuilderStateArgsList.empty()) {
     llvm::raw_string_ostream nonBuilderStateArgsOS(nonBuilderStateArgs);
-    interleaveComma(nonBuilderStateArgsList, nonBuilderStateArgsOS);
+    interleave(
+        nonBuilderStateArgsList,
+        [&](StringRef name) {
+          nonBuilderStateArgsOS << "std::forward<decltype(" << name << ")>("
+                                << name << ')';
+        },
+        [&] { nonBuilderStateArgsOS << ", "; });
+
     nonBuilderStateArgs = ", " + nonBuilderStateArgs;
   }
   if (cWithLoc)