-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[MetaSchedule] Fix thread bindings of MultiLevelTilingTensorCore #13243
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
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
shingjan
left a comment
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.
LGTM. Thanks!
junrushao
left a comment
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.
LGTM!
|
BTW, i was curious why it's not caught by the unittests of search space generation? |
|
the unit test doesn't use default config here |
|
we might want to add a unittest to cover the rule, but i understand it's kind of urgent. please set a reminder to add a regression test :-) |
|
@tvm-bot rerun |
| if (tile_binds.defined()) { | ||
| for (const String& tile_bind : tile_binds.value()) { | ||
| CHECK_NE(tile_bind, "threadIdx.x") << "Cannot bind to threadIdx.x when using tensor core."; | ||
| } | ||
| } |
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.
Does this check (along with existing tests) now cover the regression this fixes? If not adding a test to protect against the regression in the future will surely save you and others time!
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.
Oops looks like @junrushao already asked my question, I trust your judgements :)
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.
Yes this check covered the broken case that incorrectly using threadIdx.x. As a follow up, I'm thinking also adding some e2e search space generation test for the default config for each target.
6c07b5f to
4d12929
Compare
Fixed #13204.
cc @junrushao