[Hexagon] Refactor to keep HexagonBuffer private to the device api #10910
Conversation
|
cc @adstraw |
| @@ -47,14 +47,6 @@ | |||
| namespace tvm { | |||
There was a problem hiding this comment.
Can we remove the now empty namespace blocks?
| if (it != this->hexagon_buffer_map_.end()) { | ||
| return it->second.get(); | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
Since the nullptr check is done after each lookup, let's move it in here as CHECK(it != this->hexagon_buffer_map_.end()).
There was a problem hiding this 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.
There was a problem hiding this 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.
| */ | ||
| template <typename... Args> | ||
| void* AllocateHexagonBuffer(Args&&... args) { | ||
| auto buf = std::unique_ptr<HexagonBuffer>(new HexagonBuffer(std::forward<Args>(args)...)); |
There was a problem hiding this comment.
Since TVM uses C++14, we can use std::make_unique<HexagonBuffer>(std::forward<Args>(args)...) instead of new.
There was a problem hiding this 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!
There was a problem hiding this comment.
LGTM. No comments beyond what @Lunderberg already mentioned. Thanks @csullivan!
|
Note that the "external" constructor for |
|
@csullivan @adstraw @Lunderberg @kparzysz-quic thanks! merged now! |
…pache#10910) * No longer return HexagonBuffer from device api. * fixup! No longer return HexagonBuffer from device api.
…pache#10910) * No longer return HexagonBuffer from device api. * fixup! No longer return HexagonBuffer from device api.
…pache#10910) * No longer return HexagonBuffer from device api. * fixup! No longer return HexagonBuffer from device api.
…pache#10910) * No longer return HexagonBuffer from device api. * fixup! No longer return HexagonBuffer from device api.
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 outside the Device API.