[Bugfix][TIR] Fix duplicate AllocateConst in CacheReadWrite schedule primitive #16660
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lunderberg
approved these changes
Feb 29, 2024
Contributor
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Looks good, and thank you for the fix!
Long-term, we may want to avoid this class of bugs by changing the const Stmt& stmt argument of InsertCacheState to Stmt body. This would avoid having two almost-but-not-quite identical variables within the same scope.
tests/python/tir-schedule/test_tir_schedule_cache_read_write.py
Outdated
Show resolved
Hide resolved
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Mar 1, 2024
Prior to this commit, the automatic `T.reads()` and `T.writes()` annotations were only generated for buffers appearing as function arguments, as `T.alloc_buffer` in a `T.block`, or as `T.match_buffer` in a `T.block`. However, inferred `T.reads()` for a buffer defined by the `"tir.BindParams"` pass would be erroneously missing. These annotations may be required for correct scheduling (see discussion in [PR#16660](apache#16660)). This commit updates the TVMScript parsing to infer `T.reads()` and `T.writes()` annotations for buffers defined with `DeclBuffer` nodes.
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Mar 1, 2024
Prior to this commit, the automatic `T.reads()` and `T.writes()` annotations were only generated for buffers appearing as function arguments, as `T.alloc_buffer` in a `T.block`, or as `T.match_buffer` in a `T.block`. However, inferred `T.reads()` for a buffer defined by the `"tir.BindParams"` pass would be erroneously missing. These annotations may be required for correct scheduling (see discussion in [PR#16660](apache#16660)). This commit updates the TVMScript parsing to infer `T.reads()` and `T.writes()` annotations for buffers defined with `DeclBuffer` nodes.
Contributor
|
@tvm-bot rerun |
tqchen
pushed a commit
that referenced
this pull request
Mar 4, 2024
Prior to this commit, the automatic `T.reads()` and `T.writes()` annotations were only generated for buffers appearing as function arguments, as `T.alloc_buffer` in a `T.block`, or as `T.match_buffer` in a `T.block`. However, inferred `T.reads()` for a buffer defined by the `"tir.BindParams"` pass would be erroneously missing. These annotations may be required for correct scheduling (see discussion in [PR#16660](#16660)). This commit updates the TVMScript parsing to infer `T.reads()` and `T.writes()` annotations for buffers defined with `DeclBuffer` nodes.
lhutton1
approved these changes
Mar 5, 2024
Contributor
lhutton1
left a comment
lhutton1
left a comment
There was a problem hiding this comment.
Thanks @Anndrey24, LGTM. Since #16663 was merged would you prefer to address the comment here or address in a follow up?
…primitive When inserting a `cache_read` / `cache_write` stage, the `tir.AllocateConst` statement would be duplicated if its body was not a `tir.SeqStmt` node (e.g. `tir.For`), leading to compilation failures. This happened because `tir.AllocateConst` and `tir.DeclBuffer` statements are always re-attached to the statement's body after the `cache_read` / `cache_write` stage is inserted in it, but the stage was being appended to the whole statement (which already contains the `tir.AllocateConst`) and not just its body, causing duplications. This commit also adds a test where the first `cache_read` stage is inserted into a statement whose body is a `tir.For`, while the second stage is added to a body that is `tir.SeqStmt` to check for regressions.
Anndrey24
force-pushed
the
fix-alloc-const
branch
from
f65e24d to
7031fd5
Compare
Contributor
Author
|
Oops, I hadn't noticed the fix was already merged. I've also removed the |
Contributor
|
@tvm-bot rerun |
1 similar comment
Contributor
Author
|
@tvm-bot rerun |
Contributor
|
Thanks @Anndrey24 @Lunderberg! |
Lunderberg
added a commit
to Lunderberg/tvm
that referenced
this pull request
Mar 12, 2024
Prior to this commit, the automatic `T.reads()` and `T.writes()` annotations were only generated for buffers appearing as function arguments, as `T.alloc_buffer` in a `T.block`, or as `T.match_buffer` in a `T.block`. However, inferred `T.reads()` for a buffer defined by the `"tir.BindParams"` pass would be erroneously missing. These annotations may be required for correct scheduling (see discussion in [PR#16660](apache#16660)). This commit updates the TVMScript parsing to infer `T.reads()` and `T.writes()` annotations for buffers defined with `DeclBuffer` nodes.
Lunderberg
pushed a commit
to Lunderberg/tvm
that referenced
this pull request
Mar 12, 2024
…primitive (apache#16660) * [Bugfix][TIR] Fix duplicate AllocateConst in CacheReadWrite schedule primitive When inserting a `cache_read` / `cache_write` stage, the `tir.AllocateConst` statement would be duplicated if its body was not a `tir.SeqStmt` node (e.g. `tir.For`), leading to compilation failures. This happened because `tir.AllocateConst` and `tir.DeclBuffer` statements are always re-attached to the statement's body after the `cache_read` / `cache_write` stage is inserted in it, but the stage was being appended to the whole statement (which already contains the `tir.AllocateConst`) and not just its body, causing duplications. This commit also adds a test where the first `cache_read` stage is inserted into a statement whose body is a `tir.For`, while the second stage is added to a body that is `tir.SeqStmt` to check for regressions. * Improve PrimFunc readability * Remove redundant `T.reads()`
thaisacs
pushed a commit
to thaisacs/tvm
that referenced
this pull request
Apr 3, 2024
Prior to this commit, the automatic `T.reads()` and `T.writes()` annotations were only generated for buffers appearing as function arguments, as `T.alloc_buffer` in a `T.block`, or as `T.match_buffer` in a `T.block`. However, inferred `T.reads()` for a buffer defined by the `"tir.BindParams"` pass would be erroneously missing. These annotations may be required for correct scheduling (see discussion in [PR#16660](apache#16660)). This commit updates the TVMScript parsing to infer `T.reads()` and `T.writes()` annotations for buffers defined with `DeclBuffer` nodes.
thaisacs
pushed a commit
to thaisacs/tvm
that referenced
this pull request
Apr 3, 2024
…primitive (apache#16660) * [Bugfix][TIR] Fix duplicate AllocateConst in CacheReadWrite schedule primitive When inserting a `cache_read` / `cache_write` stage, the `tir.AllocateConst` statement would be duplicated if its body was not a `tir.SeqStmt` node (e.g. `tir.For`), leading to compilation failures. This happened because `tir.AllocateConst` and `tir.DeclBuffer` statements are always re-attached to the statement's body after the `cache_read` / `cache_write` stage is inserted in it, but the stage was being appended to the whole statement (which already contains the `tir.AllocateConst`) and not just its body, causing duplications. This commit also adds a test where the first `cache_read` stage is inserted into a statement whose body is a `tir.For`, while the second stage is added to a body that is `tir.SeqStmt` to check for regressions. * Improve PrimFunc readability * Remove redundant `T.reads()`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When inserting a
cache_read/cache_writestage, thetir.AllocateConststatement would be duplicated if its body was not atir.SeqStmtnode (e.g.tir.For), leading to compilation failures. This happened becausetir.AllocateConstandtir.DeclBufferstatements are always re-attached to the statement's body after thecache_read/cache_writestage is inserted in it, but the stage was being appended to the whole statement (which already contains thetir.AllocateConst) and not just its body, causing duplications.This commit also adds a test where the first
cache_readstage is inserted into a statement whose body is atir.For, while the second stage is added to a body that istir.SeqStmtto check for regressions.cc @Lunderberg @ekalda @lhutton1