Lunderberg commented on code in PR #13028:
URL: https://github.com/apache/tvm/pull/13028#discussion_r992754167
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -115,13 +118,37 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t
nbytes, size_t alignme
if (alignment < kHexagonAllocAlignment) {
alignment = kHexagonAllocAlignment;
}
- return mgr->AllocateHexagonBuffer(nbytes, alignment, String("global"));
+ CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized";
Review Comment:
Same comment here, regarding error message.
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -90,18 +90,21 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, int
ndim, const int64_t* shap
const size_t typesize = (dtype.bits / 8) * dtype.lanes;
+ CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized";
Review Comment:
This error message may be confusing to somebody encountering it for the
first time. Is the rephrasing below accurate?
```c++
CHECK(runtime_hexbuffs) << "Attempted to allocate Hexagon data with
HexagonDeviceAPI::AllocDataSpace "
<< "before initializing resources. "
<< "Please call HexagonDeviceAPI::AcquireResources";
```
##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -190,16 +194,15 @@ class HexagonDeviceAPI final : public DeviceAPI {
(DLDeviceType(dev.device_type) == kDLCPU);
}
- //! \brief Manages underlying HexagonBuffer allocations
+ //! \brief Manages RPC HexagonBuffer allocations
+ // rpc_hexbuffs is used only in Alloc/FreeRpcDataSpace
+ std::unique_ptr<HexagonBufferManager> rpc_hexbuffs;
Review Comment:
Since `rpc_hexbuffs` is initialized in the constructor and is never freed,
does it need to be a `std::unique_ptr<HexagonBufferManager>` instead of
`HexagonBufferManager`?
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -115,13 +118,37 @@ void* HexagonDeviceAPI::AllocDataSpace(Device dev, size_t
nbytes, size_t alignme
if (alignment < kHexagonAllocAlignment) {
alignment = kHexagonAllocAlignment;
}
- return mgr->AllocateHexagonBuffer(nbytes, alignment, String("global"));
+ CHECK(runtime_hexbuffs) << "runtime_hexbuffs is not initalized";
+ return runtime_hexbuffs->AllocateHexagonBuffer(nbytes, alignment,
String("global"));
}
void HexagonDeviceAPI::FreeDataSpace(Device dev, void* ptr) {
CHECK(ptr) << "buffer pointer is null";
CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type;
- mgr->FreeHexagonBuffer(ptr);
+ if (runtime_hexbuffs) {
+ runtime_hexbuffs->FreeHexagonBuffer(ptr);
+ } else {
+ LOG(INFO) << "FreeDataSpace called when runtime_hexbuffs is not
initialized";
Review Comment:
Why is this only a `LOG(INFO)`, where an uninitialized `runtime_hexbuffs` is
an error elsewhere?
##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -190,16 +194,15 @@ class HexagonDeviceAPI final : public DeviceAPI {
(DLDeviceType(dev.device_type) == kDLCPU);
}
- //! \brief Manages underlying HexagonBuffer allocations
+ //! \brief Manages RPC HexagonBuffer allocations
+ // rpc_hexbuffs is used only in Alloc/FreeRpcDataSpace
+ std::unique_ptr<HexagonBufferManager> rpc_hexbuffs;
+
+ //! \brief Manages runtime HexagonBuffer allocations
Review Comment:
It looks like the primary purpose of the separation between
`runtime_hexbuffs` and `rpc_hexbuffs` is to allow scoped allocations that can
be uniformly cleaned up at the end of a scope. Can we add a comment describing
the purpose, along with why it is only being implemented for Hexagon?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
