[Hexagon] Add fix for vtcm allocation searches #13197
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment. Generated by tvm-bot |
nverke
force-pushed
the
vtcm_alloc_fix
branch
from
2f9b770 to
8991ca1
Compare
nverke
changed the title
| ptr4 = vtcm_pool->Allocate(max_bytes); | ||
| vtcm_pool->Free(ptr4, max_bytes); | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggest:
TEST_F(HexagonVtcmPoolTest, find_allocation) {
void* ptr1;
void* ptr2;
void* ptr3;
ptr1 = vtcm_pool->Allocate(two_k_block);
ptr2 = vtcm_pool->Allocate(two_k_block);
vtcm_pool->Free(ptr1, two_k_block);
ptr3 = vtcm_pool->Allocate(four_k_block);
vtcm_pool->Free(ptr2, two_k_block);
vtcm_pool->Free(ptr3, four_k_block);
// Make sure at the end we have the full amount available again
ptr1 = vtcm_pool->Allocate(max_bytes);
vtcm_pool->Free(ptr1, max_bytes);
}
There was a problem hiding this comment.
Confirmed this fails without the fix, works great with the fix.
There was a problem hiding this comment.
Looks great - I like the new test cases!
| @@ -85,7 +85,7 @@ void* HexagonVtcmPool::Allocate(size_t nbytes) { | |||
|
|
|||
| auto entry_to_allocate = free_.begin(); | |||
| for (auto it = free_.begin(); it != free_.end(); it++) { | |||
| if ((it->second < entry_to_allocate->second) && (it->second >= nbytes)) { | |||
| if (((entry_to_allocate->second < nbytes) || (it->second < entry_to_allocate->second)) && (it->second >= nbytes)) { | |||
There was a problem hiding this comment.
The groups should be slightly adjusted:
if ((entry_to_allocate->second < nbytes) || ((it->second < entry_to_allocate->second) && (it->second >= nbytes))) {
There was a problem hiding this comment.
Doesnt this lead to us updating entry_to_allocate for iterator values that may not fit the desired allocation?
There was a problem hiding this comment.
But, that's ok because it will still find a free block that works if there is one. But on reflection, I like your grouping better - I think it makes it more clear for future editors.
| ptr4 = vtcm_pool->Allocate(max_bytes); | ||
| vtcm_pool->Free(ptr4, max_bytes); | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks great - I like the new test cases!
| vtcm_pool->Free(ptr1, two_k_block); | ||
|
|
||
| // Allocate a new larger block to initiate search and ensure | ||
| // it succeeds despite there not being a matching block. |
There was a problem hiding this comment.
nit: "despite there not being a match in the first free block."
* [Hexagon] Add fix for VTCM allocation search and new test case to cover the issue. * Add another test for a specific case. * [Hexagon] Fix tests. * Remove unused var * Change to comments to make more clear.
* [Hexagon] Add fix for VTCM allocation search and new test case to cover the issue. * Add another test for a specific case. * [Hexagon] Fix tests. * Remove unused var * Change to comments to make more clear.
Previously we were failing allocations in an attempt to find the smallest allocation possible but were failing to find an allocation that worked if it was available.