[Hexagon] Add HexagonThreadManager #11653

adstraw · 2022-06-09T20:33:27Z

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 Dispatch work including basic Signal and Wait functionality 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

- 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).

kparzysz-quic

Could you move the buffer manager changes to a separate PR? They seem independent from the thread manager.

src/runtime/hexagon/hexagon_buffer_manager.h

src/runtime/hexagon/hexagon_device_api.cc

kparzysz-quic · 2022-06-09T21:13:11Z

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?

adstraw · 2022-06-09T23:46:16Z

Could you move the buffer manager changes to a separate PR? They seem independent from the thread manager.

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.

kparzysz-quic

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 the reinterpret_cast<void*>(...) are unnecessary.
Please use nullptr instead of reinterpret_cast<...>(0).
Please avoid C-style casts, i.e. (type)value. In particular, please replace (TVMStreamHandle)i with static_cast<TVMStreamHandle>(i).
In several cases, there is code that constructs a message for DBG/DLOG that will execute unconditionally, even if DLOG is 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).

src/runtime/hexagon/hexagon_thread_manager.h

kparzysz-quic

The (TVMStreamHandle)i is still there. It's not that much of a big deal, but it would be nice to replace it with static_cast eventually.

kparzysz-quic · 2022-06-10T20:38:54Z

Thanks Adam!

adstraw · 2022-06-10T20:41:21Z

The (TVMStreamHandle)i is still there. It's not that much of a big deal, but it would be nice to replace it with static_cast eventually.

Missed this. Will fix. [Edit] We need reinterpret_cast to cast between unsigned and pointer type.
[Edit] Also missed the comment about member_variables_. Will fix that too.

adstraw · 2022-06-10T21:43:50Z

Just realized that I named my commit static cast for TVMStreamHandle before I realized that reinterpret_cast was required. I can rename that commit or ...

Just thinking out loud here ... the other thing we can do RE TVMStreamHandle is just remove it from this PR. It's there because we are looking ahead to working with actual stream handles at the Device API level. But, what we really need is just an unsigned thread ID. What's there is a bit of a misuse of a pointer type to stash an integer and probably over-engineered at least for the time being.

kparzysz-quic · 2022-06-10T21:50:44Z

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...
When the CI passes, I can change the entry in the commit message to read "reinterpret" instead of "static" right before committing.

adstraw · 2022-06-10T21:53:01Z

Just a reminder that CI is failing because it requires this PR #11635

JosephTheOctonaut and others added 30 commits June 9, 2022 10:28

Adding initial threadmanager class

5abca09

Fixed compile errors

68a3a74

Moving constant defines inside class

ee8d191

Updating qurt includes

53efd21

use default scope for hexagon buffers

ca68c77

Updating buffer allocations

691daba

Fixed bug where array of pointers treated as array of structs

12864d6

Adding initial ThreadManager tests

0a78160

HexagonThreadManager tests and refactor

ad80f5c

remove stack / pipe size member vars

c6afee0

init pointers in the header file

842e2c8

move all mem allocs to SpawnThreads

e87c969

start_semaphore as object instead of pointer

8fa3043

fix bug with WaitOnThreads deadlock + Wait/Signal off by one error

f7a7da1

add / refactor Signal / Wait tests

96190ad

add SyncFromTo test cases

6ddc75e

add Dispatch test cases

39e08a2

add pipe fill and overflow cases

31af076

Updating dispatch to return bool and fix pipe overflow problem

a13b8b1

change around min / max values for stack / pipe

366c921

integrate pipe fill / overflow tests back into HTM test suite

d30d3ab

use HexagonBuffer

b5fa501

assert if stack / pipe sizes fall below min

92213a6

Changed semaphore vector to store pointers, not structs (fixes vector…

e6ef7b9

… capacity adjustment invaliding in-use addresses).

add producer consumer, thread order test cases

70c4918

change to unordered_map for semaphores and remove PreallocateSyncs

2ee1b7d

tests running on device

88c7c0a

code cleanup for compile warnings

65e3129

adstraw added 8 commits June 9, 2022 13:17

remove redundant thread manager tests

56c91ca

revert Hex Dev API changes for threading

6b3b4c5

add comments; remove untested code to dispatch / wrap a packed func

b7ef394

pass pipe address and not HTM pointer to thread context

5461ae4

rename to HexagonBufferManager

e74ae69

cleanup ahead of PR

1a9cdfa

use DLOG(INFO)

ebc6f6a

refactor GetStreamHandles to return a vector by value

b8ae415

kparzysz-quic reviewed Jun 9, 2022

View reviewed changes

src/runtime/hexagon/hexagon_buffer_manager.h Outdated Show resolved Hide resolved

src/runtime/hexagon/hexagon_buffer_manager.h Outdated Show resolved Hide resolved

src/runtime/hexagon/hexagon_device_api.cc Outdated Show resolved Hide resolved

adjust HexagonBufferManager methods; use thread_manager file names

be1426e

github-actions bot requested a review from mehrdadh June 9, 2022 23:38

adstraw requested a review from kparzysz-quic June 9, 2022 23:39

kparzysz-quic reviewed Jun 10, 2022

View reviewed changes

src/runtime/hexagon/hexagon_thread_manager.h Outdated Show resolved Hide resolved

src/runtime/hexagon/hexagon_thread_manager.h Outdated Show resolved Hide resolved

style guidelines and debug prints

3cdc645

adstraw requested a review from kparzysz-quic June 10, 2022 20:30

kparzysz-quic approved these changes Jun 10, 2022

View reviewed changes

adstraw added 2 commits June 10, 2022 14:33

static cast for TVMStreamHandle

54d092f

end member variables with underscore

eeecedf

adstraw mentioned this pull request Jun 10, 2022

[Hexagon] Tighten requirements on inclusion of runtime sources #11635

Merged

Merge remote-tracking branch 'upstream/main' into hex-thread-mgr

9e8c7e4

kparzysz-quic merged commit 76b9ce9 into apache:main Jun 13, 2022

adstraw deleted the hex-thread-mgr branch June 13, 2022 17:51

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hexagon] Add HexagonThreadManager #11653

[Hexagon] Add HexagonThreadManager #11653

adstraw commented Jun 9, 2022 •

edited by github-actions bot

Loading

kparzysz-quic left a comment

kparzysz-quic commented Jun 9, 2022

adstraw commented Jun 9, 2022

kparzysz-quic left a comment

kparzysz-quic left a comment

kparzysz-quic commented Jun 10, 2022

adstraw commented Jun 10, 2022 •

edited

Loading

adstraw commented Jun 10, 2022 •

edited

Loading

kparzysz-quic commented Jun 10, 2022

adstraw commented Jun 10, 2022

[Hexagon] Add HexagonThreadManager #11653

[Hexagon] Add HexagonThreadManager #11653

Conversation

adstraw commented Jun 9, 2022 • edited by github-actions bot Loading

kparzysz-quic left a comment

Choose a reason for hiding this comment

kparzysz-quic commented Jun 9, 2022

adstraw commented Jun 9, 2022

kparzysz-quic left a comment

Choose a reason for hiding this comment

kparzysz-quic left a comment

Choose a reason for hiding this comment

kparzysz-quic commented Jun 10, 2022

adstraw commented Jun 10, 2022 • edited Loading

adstraw commented Jun 10, 2022 • edited Loading

kparzysz-quic commented Jun 10, 2022

adstraw commented Jun 10, 2022

adstraw commented Jun 9, 2022 •

edited by github-actions bot

Loading

adstraw commented Jun 10, 2022 •

edited

Loading

adstraw commented Jun 10, 2022 •

edited

Loading