janetsc commented on code in PR #13109:
URL: https://github.com/apache/tvm/pull/13109#discussion_r998438220
##########
src/runtime/hexagon/hexagon_device_api.cc:
##########
@@ -167,10 +149,7 @@ void* HexagonDeviceAPI::AllocWorkspace(Device dev, size_t
size, DLDataType type_
void HexagonDeviceAPI::FreeWorkspace(Device dev, void* data) {
CHECK(IsValidDevice(dev)) << "dev.device_type: " << dev.device_type;
- CHECK(runtime_hexbuffs) << "Attempted to free Hexagon workspace with "
- << "HexagonDeviceAPI::FreeWorkspace outside of a
session. "
- << "Please call HexagonDeviceAPI::AcquireResources";
- CHECK(runtime_hexbuffs->count(data) != 0)
+ CHECK(runtime_hexbuffs.count(data) != 0)
Review Comment:
This has the side effect that it will fail for things that were released in
Release(). It isn't as clear that's the case with this refactor.
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -40,14 +40,19 @@ class HexagonBufferManager {
* \param ptr Address of the HexagonBuffer as returned by
`AllocateHexagonBuffer`.
*/
void FreeHexagonBuffer(void* ptr) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ // Check the list of buffers that were still allocated at the time of
release. If this buffer
+ // is in that list, this is a no-op as it is being freed as part of
another object teardown.
+ auto it_released = std::find(released_buffers_.begin(),
released_buffers_.end(), ptr);
+ if (it_released != released_buffers_.end()) {
+ released_buffers_.erase(it_released);
+ return;
+ }
Review Comment:
I think what we want here is to not allow any new allocations when
released_buffers_ has any values.
In the current checked in code, that's what would happen because there would
be no runtime_hexbuffs
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
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() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
+ }
+
+ void Release() {
for (const auto& [data_ptr, buffer] : hexagon_buffer_map_) {
- allocated.push_back(data_ptr);
+ FreeHexagonBuffer(data_ptr);
+ {
+ std::lock_guard<std::mutex> lock(mutex_);
+ released_buffers_.push_back(data_ptr);
+ }
}
- return allocated;
}
private:
//! \brief Contains the HexagonBuffer objects managed by this class.
std::unordered_map<void*, std::unique_ptr<HexagonBuffer>>
hexagon_buffer_map_;
+ //! \brief Contains pointers to HexagonBuffer objects that had not been
freed at time of release
+ std::vector<void*> released_buffers_;
Review Comment:
What is std::unordered_set under the covers? Is it an array?
##########
src/runtime/hexagon/hexagon_buffer_manager.h:
##########
@@ -82,28 +87,35 @@ class HexagonBufferManager {
//! \brief Returns whether the HexagonBufferManager has any allocations.
bool empty() {
- std::lock_guard<std::mutex> lock(map_mutex_);
+ std::lock_guard<std::mutex> lock(mutex_);
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() {
- std::vector<void*> allocated;
- std::lock_guard<std::mutex> lock(map_mutex_);
+ void Reset() {
+ std::lock_guard<std::mutex> lock(mutex_);
+ hexagon_buffer_map_.clear();
+ released_buffers_.clear();
Review Comment:
Yes - once we call AcquireResources, we are starting a new session.
Everything we had previously tracked should be forgotten.
--
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]
