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/05/09 22:34:15 UTC
[GitHub] [tvm] csullivan opened a new pull request, #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
csullivan opened a new pull request, #11254:
URL: https://github.com/apache/tvm/pull/11254
If a memory info is not defined for a used memory scope, storage_rewrite's current behavior is undefined and will segfault.
--
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] csullivan merged pull request #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
csullivan merged PR #11254:
URL: https://github.com/apache/tvm/pull/11254
--
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 #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #11254:
URL: https://github.com/apache/tvm/pull/11254#discussion_r868578233
##########
src/tir/transforms/storage_rewrite.cc:
##########
@@ -661,9 +661,11 @@ class StoragePlanRewriter : public StmtExprMutator {
e->allocs[0]->condition, Evaluate(0));
if (IsSpecialTaggedMemory(e->scope)) {
MemoryInfo info = GetMemoryInfo(e->scope.to_string());
- uint64_t total_elem = e->const_nbits / e->elem_type.bits();
- ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
- << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ if (info.defined()) {
+ uint64_t total_elem = e->const_nbits / e->elem_type.bits();
+ ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
+ << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ }
Review Comment:
shall we throw a warning / error if memory info is not defined?
--
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] csullivan commented on a diff in pull request #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #11254:
URL: https://github.com/apache/tvm/pull/11254#discussion_r869378345
##########
src/tir/transforms/storage_rewrite.cc:
##########
@@ -661,9 +661,11 @@ class StoragePlanRewriter : public StmtExprMutator {
e->allocs[0]->condition, Evaluate(0));
if (IsSpecialTaggedMemory(e->scope)) {
MemoryInfo info = GetMemoryInfo(e->scope.to_string());
- uint64_t total_elem = e->const_nbits / e->elem_type.bits();
- ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
- << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ if (info.defined()) {
+ uint64_t total_elem = e->const_nbits / e->elem_type.bits();
+ ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
+ << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ }
Review Comment:
As structured, MemoryInfo is not well defined for Nd-memory like global.texture. If MemoryInfo is a concept we want to keep around (it's being used in two places and there exists only one definition of a MemoryInfo in the repo), then we should consider an additional PR to update its semantics. In it's current state, a warning would be applicable, I did not do that because MemoryInfo is used in other places in storage rewrite without issuing a warning when undefined and guarded with `if (info.defined()) {`.
I can update GetMemoryInfo to issue a warning when it returns a default constructed MemoryInfo().
--
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 #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #11254:
URL: https://github.com/apache/tvm/pull/11254#discussion_r869475957
##########
src/tir/transforms/storage_rewrite.cc:
##########
@@ -661,9 +661,11 @@ class StoragePlanRewriter : public StmtExprMutator {
e->allocs[0]->condition, Evaluate(0));
if (IsSpecialTaggedMemory(e->scope)) {
MemoryInfo info = GetMemoryInfo(e->scope.to_string());
- uint64_t total_elem = e->const_nbits / e->elem_type.bits();
- ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
- << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ if (info.defined()) {
+ uint64_t total_elem = e->const_nbits / e->elem_type.bits();
+ ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
+ << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ }
Review Comment:
That makes sense, thanks for the explanation. I don’t have a preference.
--
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] kparzysz-quic commented on a diff in pull request #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #11254:
URL: https://github.com/apache/tvm/pull/11254#discussion_r869721644
##########
src/tir/transforms/storage_rewrite.cc:
##########
@@ -661,9 +661,11 @@ class StoragePlanRewriter : public StmtExprMutator {
e->allocs[0]->condition, Evaluate(0));
if (IsSpecialTaggedMemory(e->scope)) {
MemoryInfo info = GetMemoryInfo(e->scope.to_string());
- uint64_t total_elem = e->const_nbits / e->elem_type.bits();
- ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
- << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ if (info.defined()) {
+ uint64_t total_elem = e->const_nbits / e->elem_type.bits();
+ ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
+ << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ }
Review Comment:
One of these places is `python/tvm/contrib/hexagon/transform.py` and it can definitely be removed from there.
--
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] csullivan commented on a diff in pull request #11254: Avoid use of MemoryInfo when undefined in StorageRewrite
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #11254:
URL: https://github.com/apache/tvm/pull/11254#discussion_r869378345
##########
src/tir/transforms/storage_rewrite.cc:
##########
@@ -661,9 +661,11 @@ class StoragePlanRewriter : public StmtExprMutator {
e->allocs[0]->condition, Evaluate(0));
if (IsSpecialTaggedMemory(e->scope)) {
MemoryInfo info = GetMemoryInfo(e->scope.to_string());
- uint64_t total_elem = e->const_nbits / e->elem_type.bits();
- ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
- << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ if (info.defined()) {
+ uint64_t total_elem = e->const_nbits / e->elem_type.bits();
+ ICHECK_LE(total_elem * e->elem_type.bits(), info->max_num_bits)
+ << "Allocation exceed bound of memory tag " << e->scope.to_string();
+ }
Review Comment:
As structured, MemoryInfo is not well defined for Nd-memory like global.texture. If MemoryInfo is a concept we want to keep around (it's being used in two places and there exists only one definition of a MemoryInfo in the repo), then we should consider an additional PR to update its semantics. In its current state, a warning would be applicable, I did not do that because MemoryInfo is used in other places in storage rewrite without issuing a warning when undefined and guarded with `if (info.defined()) {`.
I can update GetMemoryInfo to issue a warning when it returns a default constructed MemoryInfo().
--
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