-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[TensorIR] [Script] adding support for opaque block #7829
Conversation
include/tvm/tir/stmt.h
Outdated
@@ -1316,6 +1316,14 @@ constexpr const char* fragment_layout = "fragment_layout"; | |||
* \brief Mark that the kernel is hand threaded and doesn't need syncs inserted | |||
*/ | |||
constexpr const char* hand_threaded = "hand_threaded"; | |||
|
|||
/*! | |||
* \brief Mark that whether a block need to be complete access region during script parsing. |
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.
* \brief Mark that whether a block need to be complete access region during script parsing. | |
* \brief Mark whether a block need to be complete access region during script parsing. |
Can you clarify this part? to be complete access region
this doesn't parse in English for me.
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.
Mark whether the auto-completer need to fill in missing access region during script parsing.
// ignore root block or blocks which already has reads/writes regions | ||
if (block->reads.empty() || block->writes.empty()) { | ||
if (mask != 0) { |
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.
Is there a reason we moved away from high level checks like this? the masking logic is way less clear from my PoV.
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.
The reason being the that the reads/write empty may not be a good indicator(since user might indicated the reads write being empty explicitly) and we need requests from the parser side.
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.
Can we potentially wrap the masking logic? I think its a bit of a leaky abstraction for end users to need to understand low level bit masking, happy to have this be a follow up item.
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.
This is an abstraction being used by the parser(during parsing), but not being used as part of standard TIR, so it might be fine here. Happy to change to two boolean flags if needed
@jroesch please take another look if you have time |
I left one more comment we can resolve async. |
* change complete tag * add parsing support for opaque block * address and add testcase * address * address
* change complete tag * add parsing support for opaque block * address and add testcase * address * address
* change complete tag * add parsing support for opaque block * address and add testcase * address * address
* change complete tag * add parsing support for opaque block * address and add testcase * address * address
In this PR, we add script support for opaque blocks.
An opaque block is a block without any block iterations and we need to specify the read/write region manually.
cc @junrushao1994 @spectrometerHBH @tqchen