adstraw commented on code in PR #13028:
URL: https://github.com/apache/tvm/pull/13028#discussion_r993952131
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -115,13 +120,34 @@ 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) << "Attempted to allocate Hexagon data with "
+ << "HexagonDeviceAPI::AllocDataSpace before
initializing resources. "
+ << "Please call HexagonDeviceAPI::AcquireResources";
+ 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 {
Review Comment:
Not sure on the value-add for this `else` case. The only thing that happens
is that we might fail in the `CHECK` if we free a buffer after we release
resources that was not in the `released_runtime_buffers`. Do we want this
behavior? What happens on-system if we just get rid of this `else` case? We
can then get rid of `released_runtime_buffers` and the need for
`HexagonBufferManager::current_allocations`. We can simply ...
```
if (runtime_hexbuffs) {
runtime_hexbuffs->FreeHexagonBuffer(ptr);
}
```
With some comments to explain why the "free" may happen after the "release".
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -85,6 +86,18 @@ class HexagonBufferManager {
return hexagon_buffer_map_.empty();
}
+ //! \brief Returns a vector of currently allocated pointers, owned by the
manager.
+ // Note - this should only be used by the device API to keep track of what
+ // was in the manager when HexagonDeviceAPI::ReleaseResources is called.
+ std::vector<void*> current_allocations() {
Review Comment:
I apologize for the post-merge feedback. But ... having this method seems
like a break in the abstraction of the `HexagonBufferManager` class, to me. If
we have to do this it feels like something else is amiss.
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
