[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.

We need some coverage tool...

mlir-opt is a testing tool though a lot of people seem to be using it directly... Maybe we can think about having a mechanism to exclude test passes from it.

Anyway, doing "unit" tests the way Python does it means running under lit and FileCheck'ing the output. It looks simpler for C++ purposes to just add a pass into mlir-opt, I remember plumbing mlir-cpu-runner and other tools through the testing infra to make them available and it wasn't fun. We also have binaries for the C API tests that do some printing for FileCheck purposes, an alternative would be to do the same per dialect. The other argument against unit tests, as in using gtest, for op construction specifically is that we would have to programmatically check the op structure, e.g., get the attribute, assert its type, check its value, etc. which is both verbose and will not be handled by mass-update we usually do on the IR and check strings.

mlir-opt is a testing tool though a lot of people seem to be using it directly... Maybe we can think about having a mechanism to exclude test passes from it.

Do you mean like this ?

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp

ftynse · 2025-11-21T08:14:32Z

mlir/lib/Dialect/Transform/IR/TransformOps.cpp

                                                     rootType, resultTypes)));
  state.attributes.append(attrs.begin(), attrs.end());
+  if (!argAttrs.empty()) {
+    SmallVector<Attribute> argAttrsVec(argAttrs.begin(), argAttrs.end());


Nit: llvm::to_vector

Thanks for the suggestion. However, I explicitly need a SmallVector here because ArrayAttr::get() expects an ArrayRef.

Using llvm::to_vector would infer the type as SmallVector, which doesn't implicitly convert to ArrayRef. The explicit constructor handles the necessary upcast

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp

colawithsauce · 2025-11-22T03:30:08Z

sorry, I deleted the main branch by mistake.

…project

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/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp Outdated Show resolved Hide resolved

mlir/unittests/Dialect/Transform/TransformNamedSequenceCreate.cpp Outdated Show resolved Hide resolved

colawithsauce added 3 commits November 21, 2025 13:59

Merge branch 'main' into main

84fb033

[mlir][transform-dialect] comment use english & add trailing new line.

298aa73

Merge branch 'main' into main

4a62f0a

colawithsauce requested a review from ftynse November 21, 2025 07:53

ftynse requested changes Nov 21, 2025

View reviewed changes

colawithsauce deleted the branch llvm:main November 22, 2025 03:28

colawithsauce closed this Nov 22, 2025

colawithsauce deleted the main branch November 22, 2025 03:28

colawithsauce restored the main branch November 22, 2025 03:29

colawithsauce reopened this Nov 22, 2025

colawithsauce and others added 3 commits November 22, 2025 11:33

Merge branch 'main' of https://mirrors.tuna.tsinghua.edu.cn/git/llvm-…

1d12517

…project

Merge branch 'llvm:main' into main

da1b5c2

[mlir][transform-dialect] Fix named sequence build op testing script.

732adc5

colawithsauce requested a review from ftynse November 22, 2025 06:45

Merge branch 'main' into main

8b0f60d

[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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colawithsauce commented Nov 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colawithsauce Nov 18, 2025 •

edited

Loading