[Hexagon][Runtime] Add QuRT thread pool backend #11018

supersat · 2022-04-14T21:57:51Z

pthreads (and thus std::thread) does not work correctly on older versions of QuRT. This PR modifies threading_backend.cc to use the QuRT thread APIs directly when building for Hexagon devices.

@kparzysz-quic @areusch @csullivan

cc @mehrdadh

… crashes

…n_variable

… crashes

…n_variable

huajsj

Thanks.

huajsj · 2022-04-19T03:08:34Z

src/runtime/threading_backend.cc

+  qurt_thread_t thread;
+  Callback f;
+  void* stack;


qurt_thread_t thread_;
Callback f_;
void* stack_;

stack_ = nullptr

thread_, f_, and stack_ have been renamed.

Now that the return value of posix_memalign is checked, stack_ is guaranteed to be initialized.

src/runtime/threading_backend.cc

…thread-pool

areusch

thanks @supersat, one question for you

areusch · 2022-04-20T00:02:16Z

src/runtime/threading_backend.cc

 #endif
 #include <algorithm>
 #include <thread>
 #define CURRENT_THREAD_HANDLE (static_cast<std::thread::native_handle_type>(0))
 namespace tvm {
 namespace runtime {
 namespace threading {
+#ifdef __hexagon__


wondering if it's possible to do this with templating instead of with #ifdef __hexagon__?

To a certain extent, although it's not clear it's worth the effort. As far as I can tell are a couple options:

Have multiple classes that implement the ThreadGroup::Impl interface -- one for pthreads, and one for QuRT threads. @csullivan was concerned that this might lead to duplicated code, making it more fragile to maintain.

Parameterizing ThreadPool::Impl on the underlying thread type. However, we'd still need #ifdefs to avoid calling SetAffinity, which isn't available on Hexagon.

Yield() also doesn't work as-is on Hexagon, so we'd need a fix for that as well.

One approach would be to introduce

template <typename ThreadType> class ThreadGroup::Impl;

and then wrap both std::thread and QuRT thread into types with a common interface that ThreadGroup::Impl calls into.

The thing I got hung up on there is the runtime dispatch. Right now it should be doable from the device type -- when it is kDLHexagon dispatch to ThreadGroup::Impl<QuRTThreadInterface> when it's kDLCPU dispatch to TheadGroup::Impl<StdThreadInterface>. However there is impetus to move Hexagon fully over to kDLCPU -- wherein we could no longer do runtime dispatch based on the device type.

Then the only other options for dispatch I see are

(1) TVM compile-time dispatch -- during codegen, do something specific based on the target
(2) Build time dispatch via either preprocessor or through changes to the build system to conditionally include one translation unit over another.

I think that abstracting thread control would be the best approach here. The runtime built for a particular target could then contain the implementation provided by that target (e.g. via template specialization), and there wouldn't be any run-time dependency on device kind.

I've refactored this PR to split up pthread and qurt threading implementations. This introduces the following changes:

ThreadGroup::Impl is now an abstract class.

A ThreadGroupImplTemplate class is introduced with common code between pthread and qurt implementations. It is a subclass of ThreadGroup::Impl. (AFAICT, you can't have a pointer to an unspecialized template class. We need a pointer to a concrete type for ThreadGroup to call.)

ThreadGroupPosixImpl now contains the bulk of the code from ThreadGroup::Impl. It inherits from ThreadGroupImplTemplatestd::thread. This is in a new file src/runtime/posix/threading_posix.cc.

ThreadGroupHexagonImpl inherits from ThreadGroupImplTemplate, which are both defined in a new file, src/runtime/hexagon/threading_hexagon.cc.

There are now two different versions of Yield()

CMakeLists.txt has been modified to either include src/runtime/posix/threading_posix.cc or src/runtime/hexagon/threading_hexagon.cc, depending on whether we're building for Hexagon.

Honestly, this seems like a pretty ugly solution, especially when threading_backend.cc is already littered with #ifdefs for various platforms. Thoughts?

@areusch @kparzysz-quic PTAL. I'm inclined to take @supersat's recommendation if this approach is too ugly. But we'd like to keep forward progress on this one. Thanks!

src/runtime/threading_backend.cc

huajsj · 2022-04-21T01:17:24Z

src/runtime/threading_backend.cc

+    CHECK_EQ(ret, QURT_EOK);
+  }
+  QuRTThread(QuRTThread&& other) : thread_(other.thread_), f_(other.f_), stack_(other.stack_) {
+    other.thread_ = 0;


although line 73 already have the logic to avoid double free, after the construction from 'Rvalue' logically still need to clear member value(stack_, f_) of 'other', because these value already not owned by "other". doing this can help the future maintains.

free(NULL) is defined to be a no-op, but I added an explicit check anyway.

src/runtime/threading_backend.cc

csullivan

LGTM, @areusch @kparzysz-quic please follow up on your above change requests. We'd like to converge on this early next week.

huajsj · 2022-04-30T01:07:47Z

include/tvm/runtime/threading_backend.h

@@ -110,6 +110,37 @@ class ThreadGroup {
  Impl* impl_;
 };

+class ThreadGroup::Impl {
+ public:
+  virtual void Join() {}


"virtual void Join() = 0" or {assert(0);} to make sure inherit class have a dedicated "Join" and get called.

huajsj · 2022-04-30T01:54:26Z

include/tvm/runtime/threading_backend.h

+  virtual void Join() {}
+  virtual int Configure(AffinityMode mode, int nthreads, bool exclude_worker0,
+                        std::vector<unsigned int> cpus) = 0;
+  virtual ~Impl() { Join(); }


seems like here is to expect ThreadGroupImplTemplate:Join get triggered, but that may not happen, because the calling is in a deconstruct function.

huajsj · 2022-04-30T01:55:40Z

include/tvm/runtime/threading_backend.h

+      if (t.joinable()) t.join();
+    }
+  }
+


~ThreadGroupImplTemplate() { Join();}

supersat · 2022-05-02T08:36:16Z

After discussing with @csullivan, I've reverted the refactoring changes. Those changes relied on separate source files being built for separate targets, but PR #11090 introduces changes that build Hexagon sources for all targets, necessitating more #ifdefs.

Eventually the build system should only build target-specific sources when building for that target. There's a lot of cruft already in TVM because this isn't the case (e.g., #ifdef WIN32 in hexagon_buffer.cc and others). However, fixing that is a non-trivial task, so we plan to leave that to a separate PR.

huajsj

code LGTM, about the refactor issue I agree with @supersat about that using a separate PR should can make the feature/commit be more clear, Thanks @supersat.

csullivan · 2022-05-03T20:56:50Z

Thanks @supersat @huajsj @areusch @kparzysz-quic, this PR is merged.

* Initial take on adding QuRT thread support to TVM's thread pool. WIP; crashes * Allocate QuRT thread stacks automatically * Remove duplicate stack in QuRTThread * Add more logging to QuRTThread * Use QuRT mutexes and condition variables * Get QuRT thread pools working perhaps * Sleep for a little bit to let race condition bugs shine through * ayeee it works! * Remove custom hexagon implementations of std::mutex and std::condition_variable * threading_backend.cc code cleanup * Formatting changes * remove hexagon debugging * Initial take on adding QuRT thread support to TVM's thread pool. WIP; crashes * Allocate QuRT thread stacks automatically * Remove duplicate stack in QuRTThread * Add more logging to QuRTThread * Use QuRT mutexes and condition variables * Get QuRT thread pools working perhaps * Sleep for a little bit to let race condition bugs shine through * ayeee it works! * Remove custom hexagon implementations of std::mutex and std::condition_variable * threading_backend.cc code cleanup * Formatting changes * remove hexagon debugging * Add hexagon thread pool test * style fixes for tests/python/contrib/test_hexagon/test_thread_pool.py * Fix some style issues * Address some reviewer comments

supersat added 26 commits April 11, 2022 12:15

Initial take on adding QuRT thread support to TVM's thread pool. WIP;…

76877fe

… crashes

Allocate QuRT thread stacks automatically

2605634

Remove duplicate stack in QuRTThread

82bad79

Add more logging to QuRTThread

cb07bd3

Use QuRT mutexes and condition variables

f360a9f

Get QuRT thread pools working perhaps

ffeaa90

Sleep for a little bit to let race condition bugs shine through

bc0ef77

ayeee it works!

e1fd5ee

Remove custom hexagon implementations of std::mutex and std::conditio…

5948675

…n_variable

threading_backend.cc code cleanup

deebe25

Formatting changes

b1e9265

remove hexagon debugging

d27746b

Initial take on adding QuRT thread support to TVM's thread pool. WIP;…

c9ce982

… crashes

Allocate QuRT thread stacks automatically

39097ab

Remove duplicate stack in QuRTThread

832c125

Add more logging to QuRTThread

a80713f

Use QuRT mutexes and condition variables

f99d32b

Get QuRT thread pools working perhaps

85bc9b4

Sleep for a little bit to let race condition bugs shine through

91d2b23

ayeee it works!

cb2104a

Remove custom hexagon implementations of std::mutex and std::conditio…

0d20028

…n_variable

threading_backend.cc code cleanup

a0bf101

Formatting changes

95e99d9

remove hexagon debugging

912ecc6

Merge branch 'main' into qurt-thread-pool

15fafc0

Add hexagon thread pool test

e4d2b14

github-actions bot requested a review from mehrdadh April 18, 2022 23:03

style fixes for tests/python/contrib/test_hexagon/test_thread_pool.py

590c1b3

huajsj reviewed Apr 19, 2022

View reviewed changes

Fix some style issues

5576fd3

Merge branch 'qurt-thread-pool' of github.com:supersat/tvm into qurt-…

69d586e

…thread-pool

areusch reviewed Apr 20, 2022

View reviewed changes

huajsj reviewed Apr 21, 2022

View reviewed changes

Address some reviewer comments

53ef297

csullivan approved these changes Apr 29, 2022

View reviewed changes

Add QuRT thread pool backend

24f14f7

huajsj reviewed Apr 30, 2022

View reviewed changes

supersat force-pushed the qurt-thread-pool branch from b4c4277 to 24f14f7 Compare May 2, 2022 04:17

huajsj approved these changes May 2, 2022

View reviewed changes

csullivan requested review from areusch and kparzysz-quic May 2, 2022 18:20

csullivan merged commit 530091a into apache:main May 3, 2022

supersat deleted the qurt-thread-pool branch May 11, 2022 20:18

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][Runtime] Add QuRT thread pool backend #11018

[Hexagon][Runtime] Add QuRT thread pool backend #11018

supersat commented Apr 14, 2022 •

edited by github-actions bot

Loading

huajsj left a comment

huajsj Apr 19, 2022

huajsj Apr 19, 2022

supersat Apr 19, 2022

areusch left a comment

areusch Apr 20, 2022

supersat Apr 22, 2022

csullivan Apr 22, 2022

csullivan Apr 22, 2022 •

edited

Loading

kparzysz-quic Apr 22, 2022

supersat Apr 26, 2022

csullivan Apr 28, 2022

huajsj Apr 21, 2022

supersat Apr 26, 2022

csullivan left a comment

huajsj Apr 30, 2022

huajsj Apr 30, 2022

huajsj Apr 30, 2022

supersat commented May 2, 2022

huajsj left a comment

csullivan commented May 3, 2022

[Hexagon][Runtime] Add QuRT thread pool backend #11018

[Hexagon][Runtime] Add QuRT thread pool backend #11018

Conversation

supersat commented Apr 14, 2022 • edited by github-actions bot Loading

huajsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csullivan Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supersat commented May 2, 2022

huajsj left a comment

Choose a reason for hiding this comment

csullivan commented May 3, 2022

supersat commented Apr 14, 2022 •

edited by github-actions bot

Loading

csullivan Apr 22, 2022 •

edited

Loading