[MLIR] Forward generated OpTy::create arguments #170012

ricejasonf · 2025-11-29T23:30:58Z

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:

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__, cdr, packs, 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:

 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__;
}

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.

llvmbot · 2025-11-29T23:31:29Z

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Jason Rice (ricejasonf)

Changes

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&lt;(ins "::mlir::Value":$cdr,
                   "::mlir::ValueRange":$packs,
                   "std::unique_ptr&lt;::mlir::Region&gt;&amp;&amp;":$body)&gt;
  ];

Which currently generates:

ExpandPacksOp ExpandPacksOp::create(::mlir::OpBuilder &amp;builder, ::mlir::Location location, ::mlir::Value cdr, ::mlir::ValueRange packs, std::unique_ptr&lt;::mlir::Region&gt;&amp;&amp;body) {
::mlir::OperationState __state__(location, getOperationName());
build(builder, __state__, cdr, packs, body);
auto __res__ = ::llvm::dyn_cast&lt;ExpandPacksOp&gt;(builder.create(__state__));
assert(__res__ &amp;&amp; "builder didn't return the right type");
return __res__;
}

With this change it will generate:

ExpandPacksOp ExpandPacksOp::create(::mlir::OpBuilder &amp;builder, ::mlir::Location location, ::mlir::Value cdr, ::mlir::ValueRange packs, std::unique_ptr&lt;::mlir::Region&gt;&amp;&amp;body) {
  ::mlir::OperationState __state__(location, getOperationName());
  build(builder, __state__, static_cast&lt;decltype(cdr)&gt;(cdr), static_cast&lt;decltype(packs)&gt;(packs), static_cast&lt;decltype(body)&gt;(body));
  auto __res__ = ::llvm::dyn_cast&lt;ExpandPacksOp&gt;(builder.create(__state__));
  assert(__res__ &amp;&amp; "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.

Full diff: https://github.com/llvm/llvm-project/pull/170012.diff

1 Files Affected:

(modified) mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp (+8-1)

diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 3b10842f2a127..4f6c70e269fee 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 << "static_cast<decltype(" << name << ")>("
+                                << name << ')';
+        },
+        [&] { nonBuilderStateArgsOS << ", "; });
+
     nonBuilderStateArgs = ", " + nonBuilderStateArgs;
   }
   if (cWithLoc)

makslevental · 2025-11-29T23:35:37Z

Sorry noob q: what does the static_cast actually accomplish here?

Another option could be to make this function a template but then it would not be hidden in the generated translation unit.

Do you mean template with param pack? If so then the intent was exactly to avoid that (hiding etc was not the goal).

github-actions · 2025-11-29T23:50:25Z

🐧 Linux x64 Test Results

7160 tests passed
595 tests skipped

ricejasonf · 2025-11-30T01:19:55Z

The static_cast<decltype(name)>(name) will cast the variable to its type preserving rvalueness. Variable name expressions are treated as lvalues by default so they are not prone to double moves.

kuhar · 2025-11-30T14:36:18Z

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

 // 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__, static_cast<decltype(a)>(a));


Wouldn't std::forward also work here? I think that's more idiomatic

I would think that the idiomatic way would be to std::move here?

Taking the snippet from the PR description:

let builders = [ OpBuilder<(ins "::mlir::Value":$cdr, "::mlir::ValueRange":$packs, "std::unique_ptr<::mlir::Region>&&":$body)> ];

I would instead write this builder as:

let builders = [ OpBuilder<(ins "::mlir::Value":$cdr, "::mlir::ValueRange":$packs, "std::unique_ptr<::mlir::Region>":$body)> ];

That is, taking the unique_ptr by value and relying on the move.

I think move could have unexpected consequences on values passed by (non-const) reference -- IMO making arguments explicitly rvalue references and forwarding is less surprising

Yeah, I will see if I can change it to std::forward. I am used to having potential auto&& placeholder types which is not possible here.

In the DAG syntax, is a type strictly required? Can they put an addtional attribute or any non-type info there? I am having trouble finding where this is specified in the documentation.

I found a comment:

// The type string is used verbatim to produce code and, therefore, must be a // valid C++ type.

If the user passes by value say T, then the std::forward<T> will cast to T&&. I think it would still be fine, but then we are casting trivial types like int to reference types (ie int&&).

I can still change it if you would like.

I think it should be std::forward<dectlype(foo)> (which will become a static cast under the hood, but IMO the intent will be more obvious)

IE, I think the semantics we want is to move when either T or T&& and not move with T&.

Oh right, that means I can use just std::unique_ptr<mlir::Region> without the && in the parameter list.

kuhar

LGTM but can you update the PR description?

ricejasonf · 2025-12-02T01:24:09Z

I edited the original description. (I think that is what you meant)

I can squash the commits if you want, but I do not have merge access so any help with that is greatly appreciated. Also, thank you for the reviewers' time on this.

makslevental · 2025-12-02T01:47:00Z

I can squash the commits if you want, but I do not have merge access so any help with that is greatly appreciated. Also, thank you for the reviewers' time on this.

We only allow squash so it's done automatically on merge. If you're ready I can merge.

joker-eph · 2025-12-02T10:55:27Z

Can you please add a test operation in the Test dialect with a std::unique_ptr by value? I'd like to see this being compiled actually instead of limiting to the lit test.

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.

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 29, 2025

makslevental requested a review from kuhar November 29, 2025 23:34

[MLIR] Forward generated OpTy::create arguments

60da413

ricejasonf force-pushed the ricejasonf/mlir-fwd-args branch from e716f03 to 60da413 Compare November 30, 2025 01:13

makslevental requested a review from jpienaar November 30, 2025 02:11

kuhar reviewed Nov 30, 2025

View reviewed changes

[MLIR] FIXUP Use std::forward for OpTy::create arguments

ab048d0

kuhar approved these changes Dec 2, 2025

View reviewed changes

makslevental merged commit cb6e196 into llvm:main Dec 2, 2025
10 checks passed

ricejasonf mentioned this pull request Dec 2, 2025

[MLIR] Test generated build functions with move-only parameter types #170391

Merged

hrotuna mentioned this pull request Dec 4, 2025

[E#194514] Pickup patches form 20.x into 21.x intel-staging/npu-compiler-llvm#9

Open

[MLIR] Forward generated OpTy::create arguments #170012

[MLIR] Forward generated OpTy::create arguments #170012

Uh oh!

Conversation

ricejasonf commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

makslevental commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🐧 Linux x64 Test Results

Uh oh!

ricejasonf commented Nov 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricejasonf Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhar left a comment

Choose a reason for hiding this comment

Uh oh!

ricejasonf commented Dec 2, 2025

Uh oh!

makslevental commented Dec 2, 2025

Uh oh!

Uh oh!

joker-eph commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ricejasonf commented Nov 29, 2025 •

edited

Loading

llvmbot commented Nov 29, 2025 •

edited

Loading

makslevental commented Nov 29, 2025 •

edited

Loading

github-actions bot commented Nov 29, 2025 •

edited

Loading

ricejasonf Dec 1, 2025 •

edited

Loading