[Hexagon] Add HexagonThreadManager #11653
Conversation
- Updated HexagonThreadManager interface to use TVMStreams - Added second `Dispatch` interfce in thread manager to use PackedFuncs - Updated thread manager to use vector for dynamic semaphore allocation - Added "#if defined(__hexagon__)" in several places to prevent compilation errors
- Fixed GetStreams not returning the streams properly - Added missing semaphore cleanup to prevent qurt kernel resource leakage - new interface functions: - Start() : now all worker threads are blocked on initialization until ThreadManager->Start() is called - WaitOnThreads() : blocking call which waits until all worker thread queues are empty - added extra debug logging - Two new basic thread tests working
… capacity adjustment invaliding in-use addresses).
|
On another note---all the thread manager files spell "threadmanager" without the underscore, while "buffer manager" has one. Could you add the underscore for consistency? |
I am basically moving this functionality from within the Hexagon Device API where it lived previously to a separate header file. Both the Hexagon Device API and the Hexagon Thread Manager need a Hexagon Buffer Manager. The alternative would be two duplicate code in two places. |
There was a problem hiding this comment.
My general feedback would be mostly about coding style. This patch doesn't conform to the style used in TVM (for example, private class member names should end with _, etc.). Besides that:
- Almost any pointer can be implicitly converted to
void*, so most (if not all) of thereinterpret_cast<void*>(...)are unnecessary. - Please use
nullptrinstead ofreinterpret_cast<...>(0). - Please avoid C-style casts, i.e.
(type)value. In particular, please replace(TVMStreamHandle)iwithstatic_cast<TVMStreamHandle>(i). - In several cases, there is code that constructs a message for
DBG/DLOGthat will execute unconditionally, even ifDLOGis disabled. Even though the constructed string is not used, the code creating it will most likely not be removed (because it can contain library calls, etc).
|
Thanks Adam! |
Missed this. Will fix. [Edit] We need reinterpret_cast to cast between |
|
Just realized that I named my commit Just thinking out loud here ... the other thing we can do RE |
|
I wouldn't worry about renaming a commit in the PR. The code looks fine. For some reason I thought that the stream handle was actually an integer... |
|
Just a reminder that CI is failing because it requires this PR #11635 |
* Adding initial threadmanager class * Fixed compile errors * Moving constant defines inside class * Updating qurt includes * use default scope for hexagon buffers * Updating buffer allocations * Fixed bug where array of pointers treated as array of structs * - Updated HexgonDeviceAPI to use HexagonThreadManager - Updated HexagonThreadManager interface to use TVMStreams - Added second `Dispatch` interfce in thread manager to use PackedFuncs - Updated thread manager to use vector for dynamic semaphore allocation - Added "#if defined(__hexagon__)" in several places to prevent compilation errors * Bug fixes + interface addition + basic thread tests - Fixed GetStreams not returning the streams properly - Added missing semaphore cleanup to prevent qurt kernel resource leakage - new interface functions: - Start() : now all worker threads are blocked on initialization until ThreadManager->Start() is called - WaitOnThreads() : blocking call which waits until all worker thread queues are empty - added extra debug logging - Two new basic thread tests working * Adding initial ThreadManager tests * HexagonThreadManager tests and refactor * remove stack / pipe size member vars * init pointers in the header file * move all mem allocs to SpawnThreads * start_semaphore as object instead of pointer * fix bug with WaitOnThreads deadlock + Wait/Signal off by one error * add / refactor Signal / Wait tests * add SyncFromTo test cases * add Dispatch test cases * add pipe fill and overflow cases * Updating dispatch to return bool and fix pipe overflow problem * change around min / max values for stack / pipe * integrate pipe fill / overflow tests back into HTM test suite * use HexagonBuffer * assert if stack / pipe sizes fall below min * Changed semaphore vector to store pointers, not structs (fixes vector capacity adjustment invaliding in-use addresses). * add producer consumer, thread order test cases * change to unordered_map for semaphores and remove PreallocateSyncs * tests running on device * code cleanup for compile warnings * remove #if defined(__hexagon__) guards * copyright, format, lint * add hexagon buffer map class * remove redundant thread manager tests * revert Hex Dev API changes for threading * add comments; remove untested code to dispatch / wrap a packed func * pass pipe address and not HTM pointer to thread context * rename to HexagonBufferManager * cleanup ahead of PR * use DLOG(INFO) * refactor GetStreamHandles to return a vector by value * adjust HexagonBufferManager methods; use thread_manager file names * style guidelines and debug prints * reinterpret cast for TVMStreamHandle * end member variables with underscore Co-authored-by: Joseph McMahan <jmcmahan@octoml.ai>
This PR was made in collaboration with @JosephTheOctonaut.
Add HexagonThreadManager class and unit testing. HexagonThreadManager can be used to spawn a given number of QURT threads on which the user can
Dispatchwork including basicSignalandWaitfunctionality to create synchronization points between threads. This PR adds the low-level HexagonThreadManager class only. Related Hexagon Device API support and codegen to come in future PRs.This PR is dependent on #11635 to pass CI.
cc @mehrdadh