-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir] [transform dialect] NamedSequenceOp build: honor arg_attrs when building #168101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5cebc48
8b2f656
105afd1
f894d7d
06d6a9d
ca0bc78
a44361c
44268b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| #include "mlir/Dialect/Transform/IR/TransformDialect.h" | ||
| #include "mlir/Dialect/Transform/IR/TransformOps.h" | ||
| #include "mlir/IR/Builders.h" | ||
| #include "mlir/IR/BuiltinAttributes.h" | ||
| #include "mlir/IR/BuiltinOps.h" | ||
| #include "mlir/IR/MLIRContext.h" | ||
| #include "gtest/gtest.h" | ||
|
|
||
| using namespace mlir; | ||
| using namespace mlir::transform; | ||
|
|
||
| TEST(NamedSequenceOpTest, ArgAttrsAreHonoredByBuilder) { | ||
| MLIRContext ctx; | ||
| ctx.loadDialect<TransformDialect>(); | ||
|
|
||
| OpBuilder builder(&ctx); | ||
| auto module = ModuleOp::create(UnknownLoc::get(&ctx)); | ||
| builder.setInsertionPointToEnd(module.getBody()); | ||
|
|
||
| Location loc = UnknownLoc::get(&ctx); | ||
|
|
||
| static constexpr StringLiteral kMainSequenceName = "__transform_main"; | ||
|
|
||
| NamedSequenceOp seqOp = builder.create<NamedSequenceOp>( | ||
| loc, | ||
| /*sym_name=*/kMainSequenceName, | ||
| /*rootType=*/builder.getType<AnyOpType>(), | ||
| /*resultType=*/TypeRange{}, | ||
| [](OpBuilder &b, Location nested, Value rootH) { | ||
| b.create<YieldOp>(nested, ValueRange()); | ||
| }, | ||
| /*args=*/ArrayRef<NamedAttribute>{}, | ||
| /*attrArgs=*/ | ||
| ArrayRef<DictionaryAttr>{ | ||
| builder.getDictionaryAttr(ArrayRef<NamedAttribute>{ | ||
| builder.getNamedAttr(TransformDialect::kArgConsumedAttrName, | ||
| builder.getUnitAttr())})}); | ||
|
|
||
| // 检查 body argument 上有没有 transform.consumed | ||
| Block &body = seqOp.getBody().front(); | ||
| ASSERT_EQ(body.getNumArguments(), 1u); | ||
|
|
||
| StringAttr arg0Name = seqOp.getArgAttrsAttrName(); | ||
| EXPECT_TRUE(arg0Name); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would hope we don't need a c++ unit-test here, can this move to an IR test?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this bug doesn't affect IR, because when parsing IR, this function However, for those user that writing an scheduler, they need to create an 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you saying that outside of this unit-test, this method is dead code in the codebase?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked: this is dead-code... I still don't think we're usually adding C++ unit-tests for covering every builders.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I was thinking something similar to Mehdi here. Well in order
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the trailing new line (and configure your code editor not to remove it). |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use English in comments.