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/12/06 19:04:18 UTC
[GitHub] [tvm] vinx13 opened a new pull request, #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
vinx13 opened a new pull request, #13565:
URL: https://github.com/apache/tvm/pull/13565
`LegalizeIndexMapDType` previously ignore pre-defined inverse map, which causes error in `TransformLayout` when the inverse index map is predefined but the arithmetic analysis fails to compute the inverse map.
cc @masahi @Lunderberg
--
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 #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13565:
URL: https://github.com/apache/tvm/pull/13565#discussion_r1041365145
##########
src/tir/schedule/primitive/layout_transformation.cc:
##########
@@ -1079,7 +1079,13 @@ IndexMap LegalizeIndexMapDType(const IndexMap& index_map, const Buffer& buf) {
return SubstituteWithDataTypeLegalization(index,
[&](const Var& var) { return var_map.Get(var); });
});
- return IndexMap(initial_indices, final_indices);
+ Optional<IndexMap> opt_inverse_index_map =
+ Downcast<Optional<IndexMap>>(index_map->inverse_index_map);
+ if (opt_inverse_index_map.defined()) {
+ IndexMap inverse_index_map = Downcast<IndexMap>(opt_inverse_index_map.value());
+ opt_inverse_index_map = LegalizeIndexMapDType(inverse_index_map, final_indices);
Review Comment:
Probably we can remove `Downcast<IndexMap>` at L1083 and L1085?
--
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 #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13565:
URL: https://github.com/apache/tvm/pull/13565#discussion_r1041369984
##########
src/tir/schedule/primitive/layout_transformation.cc:
##########
@@ -1079,7 +1079,12 @@ IndexMap LegalizeIndexMapDType(const IndexMap& index_map, const Buffer& buf) {
return SubstituteWithDataTypeLegalization(index,
[&](const Var& var) { return var_map.Get(var); });
});
- return IndexMap(initial_indices, final_indices);
+ Optional<IndexMap> opt_inverse_index_map =
+ Downcast<Optional<IndexMap>>(index_map->inverse_index_map);
+ if (opt_inverse_index_map.defined()) {
+ opt_inverse_index_map = LegalizeIndexMapDType(opt_inverse_index_map.value(), final_indices);
Review Comment:
Can we guarantee that this recursive call would always bottom out?
--
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] vinx13 commented on a diff in pull request #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13565:
URL: https://github.com/apache/tvm/pull/13565#discussion_r1041368715
##########
src/tir/schedule/primitive/layout_transformation.cc:
##########
@@ -1079,7 +1079,13 @@ IndexMap LegalizeIndexMapDType(const IndexMap& index_map, const Buffer& buf) {
return SubstituteWithDataTypeLegalization(index,
[&](const Var& var) { return var_map.Get(var); });
});
- return IndexMap(initial_indices, final_indices);
+ Optional<IndexMap> opt_inverse_index_map =
+ Downcast<Optional<IndexMap>>(index_map->inverse_index_map);
+ if (opt_inverse_index_map.defined()) {
+ IndexMap inverse_index_map = Downcast<IndexMap>(opt_inverse_index_map.value());
+ opt_inverse_index_map = LegalizeIndexMapDType(inverse_index_map, final_indices);
Review Comment:
updated, I'm keeping L1083 as `inverse_index_map` is defined as `Optional<ObjectRef>`
--
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] vinx13 commented on a diff in pull request #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13565:
URL: https://github.com/apache/tvm/pull/13565#discussion_r1041371916
##########
src/tir/schedule/primitive/layout_transformation.cc:
##########
@@ -1079,7 +1079,12 @@ IndexMap LegalizeIndexMapDType(const IndexMap& index_map, const Buffer& buf) {
return SubstituteWithDataTypeLegalization(index,
[&](const Var& var) { return var_map.Get(var); });
});
- return IndexMap(initial_indices, final_indices);
+ Optional<IndexMap> opt_inverse_index_map =
+ Downcast<Optional<IndexMap>>(index_map->inverse_index_map);
+ if (opt_inverse_index_map.defined()) {
+ opt_inverse_index_map = LegalizeIndexMapDType(opt_inverse_index_map.value(), final_indices);
Review Comment:
It's callers' responsibility to avoid circular reference. In typical use cases, this only involves one level of recursion for the inverse map
--
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] mehrdadh commented on pull request #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #13565:
URL: https://github.com/apache/tvm/pull/13565#issuecomment-1340108054
Please rebase because of https://github.com/apache/tvm/commit/9374738b29c986e863d331ffe29e1b36828a8565
--
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] vinx13 merged pull request #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
vinx13 merged PR #13565:
URL: https://github.com/apache/tvm/pull/13565
--
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 #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13565:
URL: https://github.com/apache/tvm/pull/13565#issuecomment-1339839812
<!---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, @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] masahi commented on a diff in pull request #13565: [TIR] Fix predefined inverse map in layout transform dtype legalization
Posted by GitBox <gi...@apache.org>.
masahi commented on code in PR #13565:
URL: https://github.com/apache/tvm/pull/13565#discussion_r1041369984
##########
src/tir/schedule/primitive/layout_transformation.cc:
##########
@@ -1079,7 +1079,12 @@ IndexMap LegalizeIndexMapDType(const IndexMap& index_map, const Buffer& buf) {
return SubstituteWithDataTypeLegalization(index,
[&](const Var& var) { return var_map.Get(var); });
});
- return IndexMap(initial_indices, final_indices);
+ Optional<IndexMap> opt_inverse_index_map =
+ Downcast<Optional<IndexMap>>(index_map->inverse_index_map);
+ if (opt_inverse_index_map.defined()) {
+ opt_inverse_index_map = LegalizeIndexMapDType(opt_inverse_index_map.value(), final_indices);
Review Comment:
Can we guarantee that this recursive call would always bottom out? i.e. Can the inverse map also stores the original forward map as its inverse map?
--
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