You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/11/15 00:52:53 UTC
[GitHub] [tvm] masahi opened a new pull request, #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
masahi opened a new pull request, #13384:
URL: https://github.com/apache/tvm/pull/13384
Currently when `CacheRead` and `RewriteLayout` are used together, the index map is derived based on the cache read block, which leads to weird result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13384:
URL: https://github.com/apache/tvm/pull/13384#issuecomment-1314607270
<!---bot-comment-->
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](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @Hzfengsy, @elvin-n, @junrushao <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1023165629
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
An alternative idea: having an extra flag in `CreatePrimFunc` that instructs the translator to retain buffers in block annotation, and the flag is on only if `link-param` is detected as "on" in TECompiler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao merged pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
junrushao merged PR #13384:
URL: https://github.com/apache/tvm/pull/13384
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1023180356
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
This `else` branch is only hit when there is a layout-free buffer that's not passed as a parameter to the prim func. Currently that can only happen when `link-params = True` (this is exactly what `link-params` is meant for). So isn't having another flag redundant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1022244412
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
This is a fix for `RewriteLayout` to work when `link-params = True`, the breakage discussed in https://github.com/apache/tvm/pull/13195. With this, I confirmed that the new `RewriteLayout` implementation works when there is `CacheRead` acting on `AllocateConst`. I'm not adding a test since TVMScript parsing is broken when there is `AllocateConst` with a large constant (can't print the whole array).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1023226878
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
I see. That makes perfect sense to me :-) As long as it's not affecting normal lowering flow, it looks like a good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1023180356
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
This `else` branch is only hit when there is a layout-free buffer that's not passed as a parameter to the prim func. Currently that can only happen when `link-params = True`. So isn't having another flag redundant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13384: [MetaSchedule] Support schedules with cache read in RewriteLayout
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13384:
URL: https://github.com/apache/tvm/pull/13384#discussion_r1023136135
##########
src/te/operation/create_primfunc.cc:
##########
@@ -110,13 +110,20 @@ class LayoutFreePlaceholdersNormalizer : public StmtMutator {
Block block = Downcast<Block>(StmtMutator::VisitStmt_(_block));
BlockNode* n = block.CopyOnWrite();
if (Optional<ObjectRef> ann = n->annotations.Get(topi_attr)) {
+ Array<Buffer> new_buffers;
for (Buffer buffer : Downcast<Array<Buffer>>(ann)) {
auto it = buffer2index_.find(buffer);
if (it != buffer2index_.end()) {
layout_free_buffer_indices_.insert(it->second);
+ } else {
+ new_buffers.push_back(buffer);
}
}
- n->annotations.erase(topi_attr);
+ if (new_buffers.empty()) {
+ n->annotations.erase(topi_attr);
+ } else {
+ n->annotations.Set(topi_attr, new_buffers);
+ }
Review Comment:
Continuing the discussion from [this thread](https://github.com/apache/tvm/pull/13195#discussion_r1022226540).
I am convinced that we need to store buffers in block attributes for hexagon-specific usecases, and the only thing that I feel less natural is generally storing IR nodes in attributes. As an example, in `layout_free_placeholders`, we didn't store any IR nodes in the attribute, but instead use a list of integers which is simpler.
In the meantime, I completely understand that we need to get around this quickly, so in this particular case, how about adding or reusing a pass config flag, e.g. the one we are using "link-params", and only add enable `topi_attr` to be set when the flag is on?
```c++
else if (link_param is on) {
n->annotations.Set(topi_attr, new_buffers);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org