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 2021/12/17 17:04:11 UTC
[GitHub] [tvm] kparzysz-quic opened a new pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
kparzysz-quic opened a new pull request #9769:
URL: https://github.com/apache/tvm/pull/9769
In particular, graph executor will reuse allocated buffers for various tensors. These tensors may be of various sizes as long as the buffer is large enough to hold them.
--
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] adstraw commented on pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
adstraw commented on pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#issuecomment-998325782
@kparzysz-quic you should be good to merge now; thanks @junrushao1994 for reviewing!
--
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] tmoreau89 commented on pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
tmoreau89 commented on pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#issuecomment-996879553
CC @csullivan @Lunderberg @adstraw
--
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] junrushao1994 commented on pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#issuecomment-998485746
Thanks @kparzysz-quic @adstraw! Please enjoy the holiday BTW!
--
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] junrushao1994 merged pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #9769:
URL: https://github.com/apache/tvm/pull/9769
--
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] adstraw commented on a change in pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
adstraw commented on a change in pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#discussion_r772505591
##########
File path: src/runtime/hexagon/hexagon/hexagon_buffer.cc
##########
@@ -187,66 +188,55 @@ void HexagonBuffer::SetStorageScope(Optional<String> scope) {
}
}
-void HexagonBuffer::CopyTo(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+void HexagonBuffer::CopyTo(void* data, size_t nbytes) const {
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(data) + offset,
- static_cast<const char*>(managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
void HexagonBuffer::CopyFrom(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(data) + offset, managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(data) + offset, bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
-void HexagonBuffer::CopyFrom(const HexagonBuffer& other) {
- CHECK(nbytes_ == other.nbytes_);
+void HexagonBuffer::CopyFrom(const HexagonBuffer& other, size_t nbytes) {
+ CHECK_LE(nbytes, nbytes_);
+ CHECK_LE(nbytes, other.nbytes_);
if (nallocs_ == other.nallocs_) {
+ size_t copied = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(managed_allocations_[i]->nbytes_ == other.managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - copied, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
- memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
- }
- } else if (nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < other.nallocs_; ++i) {
- CHECK(nbytes_ / other.nallocs_ == other.managed_allocations_[i]->nbytes_);
-
- memcpy(static_cast<char*>(managed_allocations_[0]->data_) + offset,
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- other.managed_allocations_[i]->nbytes_);
-
- offset += other.managed_allocations_[i]->nbytes_;
- }
- } else if (other.nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(other.nbytes_ / nallocs_ == managed_allocations_[i]->nbytes_);
+ CHECK_LE(other.managed_allocations_[i]->nbytes_, managed_allocations_[i]->nbytes_);
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[0]->data_) + offset,
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(other.managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ copied += bytes_to_copy;
Review comment:
Nit: It's called `copied` here and `offset` in `CopyTo` and `CopyFrom` above. The concept is the exact same, I believe, but the variable names are different which could cause some confusion. I don't have a preference on which name so long as it's consistent.
##########
File path: src/runtime/hexagon/hexagon/hexagon_buffer.cc
##########
@@ -187,66 +188,55 @@ void HexagonBuffer::SetStorageScope(Optional<String> scope) {
}
}
-void HexagonBuffer::CopyTo(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+void HexagonBuffer::CopyTo(void* data, size_t nbytes) const {
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(data) + offset,
- static_cast<const char*>(managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
void HexagonBuffer::CopyFrom(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(data) + offset, managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(data) + offset, bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
-void HexagonBuffer::CopyFrom(const HexagonBuffer& other) {
- CHECK(nbytes_ == other.nbytes_);
+void HexagonBuffer::CopyFrom(const HexagonBuffer& other, size_t nbytes) {
+ CHECK_LE(nbytes, nbytes_);
+ CHECK_LE(nbytes, other.nbytes_);
if (nallocs_ == other.nallocs_) {
+ size_t copied = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(managed_allocations_[i]->nbytes_ == other.managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - copied, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
- memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
- }
- } else if (nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < other.nallocs_; ++i) {
- CHECK(nbytes_ / other.nallocs_ == other.managed_allocations_[i]->nbytes_);
-
- memcpy(static_cast<char*>(managed_allocations_[0]->data_) + offset,
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- other.managed_allocations_[i]->nbytes_);
-
- offset += other.managed_allocations_[i]->nbytes_;
- }
- } else if (other.nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(other.nbytes_ / nallocs_ == managed_allocations_[i]->nbytes_);
+ CHECK_LE(other.managed_allocations_[i]->nbytes_, managed_allocations_[i]->nbytes_);
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[0]->data_) + offset,
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(other.managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ copied += bytes_to_copy;
}
+ } else if (nallocs_ == 1) {
Review comment:
Good catch and change!
--
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 change in pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#discussion_r772655068
##########
File path: src/runtime/hexagon/hexagon/hexagon_buffer.cc
##########
@@ -187,66 +188,55 @@ void HexagonBuffer::SetStorageScope(Optional<String> scope) {
}
}
-void HexagonBuffer::CopyTo(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+void HexagonBuffer::CopyTo(void* data, size_t nbytes) const {
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(data) + offset,
- static_cast<const char*>(managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
void HexagonBuffer::CopyFrom(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(data) + offset, managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(data) + offset, bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
-void HexagonBuffer::CopyFrom(const HexagonBuffer& other) {
- CHECK(nbytes_ == other.nbytes_);
+void HexagonBuffer::CopyFrom(const HexagonBuffer& other, size_t nbytes) {
+ CHECK_LE(nbytes, nbytes_);
+ CHECK_LE(nbytes, other.nbytes_);
if (nallocs_ == other.nallocs_) {
+ size_t copied = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(managed_allocations_[i]->nbytes_ == other.managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - copied, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
- memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
- }
- } else if (nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < other.nallocs_; ++i) {
- CHECK(nbytes_ / other.nallocs_ == other.managed_allocations_[i]->nbytes_);
-
- memcpy(static_cast<char*>(managed_allocations_[0]->data_) + offset,
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- other.managed_allocations_[i]->nbytes_);
-
- offset += other.managed_allocations_[i]->nbytes_;
- }
- } else if (other.nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(other.nbytes_ / nallocs_ == managed_allocations_[i]->nbytes_);
+ CHECK_LE(other.managed_allocations_[i]->nbytes_, managed_allocations_[i]->nbytes_);
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[0]->data_) + offset,
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(other.managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ copied += bytes_to_copy;
Review comment:
Apparently this PR still needs approval from someone else, so I just added another commit that renames `offset` to `copied`.
--
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 pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#issuecomment-998021099
The original checks for equal sizes break most of our full model tests downstream (due to buffer reuse in graph executor). @adstraw: could you take a look at this?
The biggest change here is that the number of bytes to copy is now required for HexagonBuffer -> HexagonBuffer copy, the rest is simply relaxing the size checks. For buffers with multiple allocations, the object payload is assumed to fill the initial allocations, leaving the tail allocations (and the tail end of a partially filled allocation) potentially unused.
--
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 change in pull request #9769: [Hexagon] Account for objects being smaller than the allocated space
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on a change in pull request #9769:
URL: https://github.com/apache/tvm/pull/9769#discussion_r772510269
##########
File path: src/runtime/hexagon/hexagon/hexagon_buffer.cc
##########
@@ -187,66 +188,55 @@ void HexagonBuffer::SetStorageScope(Optional<String> scope) {
}
}
-void HexagonBuffer::CopyTo(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+void HexagonBuffer::CopyTo(void* data, size_t nbytes) const {
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(data) + offset,
- static_cast<const char*>(managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
void HexagonBuffer::CopyFrom(void* data, size_t nbytes) {
- CHECK(nbytes_ == nbytes);
+ CHECK_LE(nbytes, nbytes_);
size_t offset = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(nbytes / nallocs_ == managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - offset, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(data) + offset, managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(data) + offset, bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ offset += bytes_to_copy;
}
}
-void HexagonBuffer::CopyFrom(const HexagonBuffer& other) {
- CHECK(nbytes_ == other.nbytes_);
+void HexagonBuffer::CopyFrom(const HexagonBuffer& other, size_t nbytes) {
+ CHECK_LE(nbytes, nbytes_);
+ CHECK_LE(nbytes, other.nbytes_);
if (nallocs_ == other.nallocs_) {
+ size_t copied = 0;
for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(managed_allocations_[i]->nbytes_ == other.managed_allocations_[i]->nbytes_);
+ size_t bytes_to_copy = std::min(nbytes - copied, managed_allocations_[i]->nbytes_);
+ if (bytes_to_copy == 0) break;
- memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- managed_allocations_[i]->nbytes_);
- }
- } else if (nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < other.nallocs_; ++i) {
- CHECK(nbytes_ / other.nallocs_ == other.managed_allocations_[i]->nbytes_);
-
- memcpy(static_cast<char*>(managed_allocations_[0]->data_) + offset,
- static_cast<const char*>(other.managed_allocations_[i]->data_),
- other.managed_allocations_[i]->nbytes_);
-
- offset += other.managed_allocations_[i]->nbytes_;
- }
- } else if (other.nallocs_ == 1) {
- size_t offset = 0;
- for (size_t i = 0; i < nallocs_; ++i) {
- CHECK(other.nbytes_ / nallocs_ == managed_allocations_[i]->nbytes_);
+ CHECK_LE(other.managed_allocations_[i]->nbytes_, managed_allocations_[i]->nbytes_);
memcpy(static_cast<char*>(managed_allocations_[i]->data_),
- static_cast<const char*>(other.managed_allocations_[0]->data_) + offset,
- managed_allocations_[i]->nbytes_);
+ static_cast<const char*>(other.managed_allocations_[i]->data_), bytes_to_copy);
- offset += managed_allocations_[i]->nbytes_;
+ copied += bytes_to_copy;
Review comment:
Yeah, I didn't want to modify the existing code too much, so I left `offset` unchanged. I'll merge it to unblock our tests, but I'll create another PR to make the names consistent. Thanks Adam!
--
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