[mlir] [transform dialect] NamedSequenceOp build: honor arg_attrs when building #168101

colawithsauce · 2025-11-14T18:29:59Z

Previously the builder did not attach argument attributes when calling NamedSequenceOp::build with arg_attrs, which makes we can not create a legal named_sequence operation by this builder.

This patch ensures arg_attrs are stored in the op state so verification and printing show them.

Previously the builder did not attach argument attributes when calling NamedSequenceOp::build with `arg_attrs`, which makes we can not create a legal named_sequence operation by this builder. This patch ensures `arg_attrs` are stored in the op state so verification and printing show them.

github-actions · 2025-11-14T18:30:21Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-11-14T18:30:56Z

@llvm/pr-subscribers-mlir

Author: None (colawithsauce)

Changes

Previously the builder did not attach argument attributes when calling NamedSequenceOp::build with arg_attrs, which makes we can not create a legal named_sequence operation by this builder.

This patch ensures arg_attrs are stored in the op state so verification and printing show them.

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

1 Files Affected:

(modified) mlir/lib/Dialect/Transform/IR/TransformOps.cpp (+5)

diff --git a/mlir/lib/Dialect/Transform/IR/TransformOps.cpp b/mlir/lib/Dialect/Transform/IR/TransformOps.cpp
index 062606e7e10b6..415e2af491c07 100644
--- a/mlir/lib/Dialect/Transform/IR/TransformOps.cpp
+++ b/mlir/lib/Dialect/Transform/IR/TransformOps.cpp
@@ -2571,6 +2571,11 @@ void transform::NamedSequenceOp::build(OpBuilder &builder,
                      TypeAttr::get(FunctionType::get(builder.getContext(),
                                                      rootType, resultTypes)));
   state.attributes.append(attrs.begin(), attrs.end());
+  if (!argAttrs.empty()) {
+    SmallVector<Attribute> argAttrsVec(argAttrs.begin(), argAttrs.end());
+    state.getOrAddProperties<Properties>().arg_attrs =
+        ArrayAttr::get(builder.getContext(), argAttrsVec);
+  }
   state.addRegion();
 
   buildSequenceBody(builder, state, rootType,

ftynse

Looks like some side effect of property transition, thanks!

joker-eph · 2025-11-17T20:01:54Z

Can this be tested?

colawithsauce · 2025-11-18T03:23:21Z

Can this be tested?

of course, I'll add an unit test soon.

colawithsauce · 2025-11-18T03:30:14Z

Can this be tested?

Now, after CMake:

cd build && ninja MLIRTransformDialectTests
./tools/mlir/unittests/Dialect/Transform/MLIRTransformDialectTests

joker-eph · 2025-11-18T13:16:49Z

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp

+
+  StringAttr arg0Name = seqOp.getArgAttrsAttrName();
+  EXPECT_TRUE(arg0Name);
+}


I would hope we don't need a c++ unit-test here, can this move to an IR test?

this bug doesn't affect IR, because when parsing IR, this function NamedSequence::build does not invoked. It invokes versions that auto generated by TableGen.

However, for those user that writing an scheduler, they need to create an transform.named_sequence with C++

this build function addtion to versions auto genrated is to make this progress convinient.

So i am afraid to say sorry, I could not write an IR test for this case, because its only for c++

thanks for your attention.

Are you saying that outside of this unit-test, this method is dead code in the codebase?

I checked: this is dead-code...

I still don't think we're usually adding C++ unit-tests for covering every builders.
But also, builders are added when needed, I guess someone had a need downstream when they added this builder?

I think so. This builder have no usage out of this unittest. In my AI compiler project I use it to build my own schedule sequence. But it failed to work. And then with hours of struggling I find this bug and fix it at my downstream.

Since it is dead code, I have no idea this should be fix or removed instead?

We don't have an established pattern for testing this kind of code, I wonder if we should have a single file per dialect, maybe: TransformDialectODSHelperTests.cpp or something like that?

@jpienaar maybe?

I don't think any of the builders are tested directly. We could test them by, e.g., having a test pass (per dialect) that simply calls the builders and FileCheck the IR generated by the pass.

Yes I was thinking something similar to Mehdi here. Well in order

Ideally builders (not automatically generated) are tested via existing passes upstream and there is use of them. No additional worked needed and builder obviously useful.

Builder file say per dialect.

The pros and cons of builder unit test vs pass for me is, that unit test becomes its own binary (so extra linkage cost) but it doesn't bloat mlir-opt. Now, one could argue that mlir-opt is already a testing tool and so that doesn't actually matter. In which case Alex's suggestion is better.

I could see us doing a C++ unit test that looks like the Python lit tests (compile, run and check with lit). I am spoilt as I have a parallel build, but if we restrict it per dialect and only for those not covered in another way already, it's probably small set and not high cost while being very directed.

ftynse · 2025-11-20T08:47:20Z

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp

+              builder.getNamedAttr(TransformDialect::kArgConsumedAttrName,
+                                   builder.getUnitAttr())})});
+
+  // 检查 body argument 上有没有 transform.consumed


Please use English in comments.

ftynse · 2025-11-20T08:47:38Z

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp

+
+  StringAttr arg0Name = seqOp.getArgAttrsAttrName();
+  EXPECT_TRUE(arg0Name);
+}


Add the trailing new line (and configure your code editor not to remove it).

colawithsauce added 2 commits November 15, 2025 02:24

Merge branch 'llvm:main' into main

8b2f656

colawithsauce requested review from ftynse, nicolasvasilache and rolfmorel as code owners November 14, 2025 18:30

llvmbot added the mlir label Nov 14, 2025

colawithsauce added 3 commits November 15, 2025 02:31

Merge branch 'main' into main

105afd1

Merge branch 'main' into main

f894d7d

Merge branch 'main' into main

06d6a9d

ftynse approved these changes Nov 17, 2025

View reviewed changes

Merge branch 'main' into main

ca0bc78

[mlir][transform-dialect] add unittest of named_sequence build.

a44361c

joker-eph reviewed Nov 18, 2025

View reviewed changes

Merge branch 'main' into main

44268b1

ftynse self-requested a review November 20, 2025 08:44

ftynse reviewed Nov 20, 2025

View reviewed changes

[mlir] [transform dialect] NamedSequenceOp build: honor arg_attrs when building #168101

Are you sure you want to change the base?

[mlir] [transform dialect] NamedSequenceOp build: honor arg_attrs when building #168101

Conversation

colawithsauce commented Nov 14, 2025

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

llvmbot commented Nov 14, 2025

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

joker-eph commented Nov 17, 2025

Uh oh!

colawithsauce commented Nov 18, 2025

Uh oh!

colawithsauce commented Nov 18, 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!

colawithsauce Nov 18, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colawithsauce Nov 18, 2025 •

edited

Loading