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/04/05 22:32:21 UTC
[GitHub] [tvm] csullivan opened a new pull request, #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
csullivan opened a new pull request, #10910:
URL: https://github.com/apache/tvm/pull/10910
Refactoring the DeviceAPI a bit to privatize the HexagonBuffer class and no longer return it across the API boundary. So far we haven't seen a need to identify the memory scope, layout, and other properties of a HexagonBuffer allocated by the Device API.
--
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 merged pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
mehrdadh merged PR #10910:
URL: https://github.com/apache/tvm/pull/10910
--
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] Lunderberg commented on a diff in pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #10910:
URL: https://github.com/apache/tvm/pull/10910#discussion_r843936368
##########
src/runtime/hexagon/hexagon/hexagon_common.h:
##########
@@ -47,14 +47,6 @@
namespace tvm {
Review Comment:
Can we remove the now empty namespace blocks?
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.h:
##########
@@ -125,8 +126,23 @@ class HexagonDeviceAPIv2 final : public DeviceAPI {
TVMStreamHandle stream) final;
private:
- //! Lookup table for the HexagonBuffer managing a workspace allocation.
- std::unordered_map<void*, HexagonBuffer*> workspace_allocations_;
+ /*! \brief Helper to allocate a HexagonBuffer and register the result
+ * in the owned buffer map.
+ * \return Raw data storage managed by the hexagon buffer
+ */
+ template <typename... Args>
+ void* AllocateHexagonBuffer(Args&&... args) {
+ auto buf = std::unique_ptr<HexagonBuffer>(new HexagonBuffer(std::forward<Args>(args)...));
Review Comment:
Since TVM uses C++14, we can use `std::make_unique<HexagonBuffer>(std::forward<Args>(args)...)` instead of `new`.
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.cc:
##########
@@ -148,20 +139,29 @@ void HexagonDeviceAPIv2::CopyDataFromTo(DLTensor* from, DLTensor* to, TVMStreamH
CHECK_EQ(to->byte_offset, 0);
CHECK_EQ(GetDataSize(*from), GetDataSize(*to));
- HexagonBuffer* hex_from_buf = static_cast<HexagonBuffer*>(from->data);
- HexagonBuffer* hex_to_buf = static_cast<HexagonBuffer*>(to->data);
+ auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* {
+ auto it = this->hexagon_buffer_map_.find(ptr);
+ if (it != this->hexagon_buffer_map_.end()) {
+ return it->second.get();
+ }
+ return nullptr;
Review Comment:
Since the `nullptr` check is done after each lookup, let's move it in here as `CHECK(it != this->hexagon_buffer_map_.end())`.
--
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 #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
adstraw commented on PR #10910:
URL: https://github.com/apache/tvm/pull/10910#issuecomment-1090434886
Note that the "external" constructor for `HexagonBuffer` using `void*` is no longer required after this PR and furthermore `HexagonBuffer` can be simplified as it no longer needs to separately track managed and unmanaged allocations. I think it's OK to follow up with a PR to do this cleanup work in `HexagonBuffer` --- just wanted to note it here.
--
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 pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
csullivan commented on PR #10910:
URL: https://github.com/apache/tvm/pull/10910#issuecomment-1089451390
cc @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] csullivan commented on a diff in pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #10910:
URL: https://github.com/apache/tvm/pull/10910#discussion_r844106727
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.h:
##########
@@ -125,8 +126,23 @@ class HexagonDeviceAPIv2 final : public DeviceAPI {
TVMStreamHandle stream) final;
private:
- //! Lookup table for the HexagonBuffer managing a workspace allocation.
- std::unordered_map<void*, HexagonBuffer*> workspace_allocations_;
+ /*! \brief Helper to allocate a HexagonBuffer and register the result
+ * in the owned buffer map.
+ * \return Raw data storage managed by the hexagon buffer
+ */
+ template <typename... Args>
+ void* AllocateHexagonBuffer(Args&&... args) {
+ auto buf = std::unique_ptr<HexagonBuffer>(new HexagonBuffer(std::forward<Args>(args)...));
Review Comment:
Alas my configuration for hexagon was giving false negatives on finding std::make_unique in the std. I had intended to circle back on this one before pushing but forgot, so thank you for the reminder!
--
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 #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #10910:
URL: https://github.com/apache/tvm/pull/10910#issuecomment-1090896256
@csullivan @adstraw @Lunderberg @kparzysz-quic thanks! merged now!
--
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 diff in pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #10910:
URL: https://github.com/apache/tvm/pull/10910#discussion_r844101727
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.cc:
##########
@@ -148,20 +139,29 @@ void HexagonDeviceAPIv2::CopyDataFromTo(DLTensor* from, DLTensor* to, TVMStreamH
CHECK_EQ(to->byte_offset, 0);
CHECK_EQ(GetDataSize(*from), GetDataSize(*to));
- HexagonBuffer* hex_from_buf = static_cast<HexagonBuffer*>(from->data);
- HexagonBuffer* hex_to_buf = static_cast<HexagonBuffer*>(to->data);
+ auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* {
+ auto it = this->hexagon_buffer_map_.find(ptr);
+ if (it != this->hexagon_buffer_map_.end()) {
+ return it->second.get();
+ }
+ return nullptr;
Review Comment:
Agree with @Lunderberg's comment and also think that `lookup_hexagon_buffer` could / should be a member of `HexagonDeviceAPIv2` named `LookupHexagonBuffer` alongside the "Allocate" and "Free" methods.
--
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] Lunderberg commented on a diff in pull request #10910: [Hexagon] Refactor to keep HexagonBuffer private to the device api
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #10910:
URL: https://github.com/apache/tvm/pull/10910#discussion_r844114261
##########
src/runtime/hexagon/hexagon/hexagon_device_api_v2.cc:
##########
@@ -148,20 +139,29 @@ void HexagonDeviceAPIv2::CopyDataFromTo(DLTensor* from, DLTensor* to, TVMStreamH
CHECK_EQ(to->byte_offset, 0);
CHECK_EQ(GetDataSize(*from), GetDataSize(*to));
- HexagonBuffer* hex_from_buf = static_cast<HexagonBuffer*>(from->data);
- HexagonBuffer* hex_to_buf = static_cast<HexagonBuffer*>(to->data);
+ auto lookup_hexagon_buffer = [this](void* ptr) -> HexagonBuffer* {
+ auto it = this->hexagon_buffer_map_.find(ptr);
+ if (it != this->hexagon_buffer_map_.end()) {
+ return it->second.get();
+ }
+ return nullptr;
Review Comment:
I like that, because that would let it be used in the `free_nd` function as well. Though, I think it should be private, unlike the "Allocate" and "Free" methods.
--
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