huajsj commented on code in PR #11018:
URL: https://github.com/apache/tvm/pull/11018#discussion_r854672421
##########
src/runtime/threading_backend.cc:
##########
@@ -276,7 +334,17 @@ int ThreadGroup::Configure(AffinityMode mode, int
nthreads, bool exclude_worker0
return impl_->Configure(mode, nthreads, exclude_worker0, cpus);
}
-void Yield() { std::this_thread::yield(); }
+void Yield() {
+#ifdef __hexagon__
+ // QuRT doesn't have a yield API, so instead we sleep for the minimum amount
+ // of time to let the OS schedule another thread. std::this_thread::yield()
+ // compiles down to an empty function.
+ qurt_sleep(1);
Review Comment:
if the expectation is to let OS to do the schedule for CPU resource, why can
not use std::this_thread::yield()? and could you help to put the information
about the time unit for '1'(1 second or 1 millisecond)? if a qurt_sleep is
necessary, does a 'qurt_sleep(0)' can trigger a yield?
##########
src/runtime/threading_backend.cc:
##########
@@ -34,13 +34,63 @@
#endif
#if defined(__hexagon__)
#include <dlfcn.h>
+#include <qurt.h>
+#include <stdlib.h>
+#define HEXAGON_STACK_SIZE 65536
+#define HEXAGON_STACK_ALIGNMENT 32
#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__
+// pthreads are broken on older versions of qurt, so
+// we need to use native APIs instead of std::threads
+class QuRTThread {
+ typedef std::function<void()> Callback;
+
+ public:
+ explicit QuRTThread(Callback worker_callback) : f_(worker_callback) {
+ static int id = 1;
+ qurt_thread_attr_t attr;
+ char name[32];
+ int ret = posix_memalign(&stack_, HEXAGON_STACK_ALIGNMENT,
HEXAGON_STACK_SIZE);
+ CHECK_EQ(ret, 0);
+ qurt_thread_attr_init(&attr);
+ qurt_thread_attr_set_stack_size(&attr, HEXAGON_STACK_SIZE);
+ qurt_thread_attr_set_stack_addr(&attr, stack_);
+ snprintf(name, sizeof(name), "worker %d", id++);
+ qurt_thread_attr_set_name(&attr, name);
+ ret = qurt_thread_create(&thread_, &attr, (void (*)(void*))RunFunction,
this);
+ CHECK_EQ(ret, QURT_EOK);
+ }
+ QuRTThread(QuRTThread&& other) : thread_(other.thread_), f_(other.f_),
stack_(other.stack_) {
+ other.thread_ = 0;
Review Comment:
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.
##########
src/runtime/threading_backend.cc:
##########
@@ -34,13 +34,63 @@
#endif
#if defined(__hexagon__)
#include <dlfcn.h>
+#include <qurt.h>
+#include <stdlib.h>
+#define HEXAGON_STACK_SIZE 65536
+#define HEXAGON_STACK_ALIGNMENT 32
#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__
+// pthreads are broken on older versions of qurt, so
+// we need to use native APIs instead of std::threads
+class QuRTThread {
+ typedef std::function<void()> Callback;
+
+ public:
+ explicit QuRTThread(Callback worker_callback) : f_(worker_callback) {
+ static int id = 1;
+ qurt_thread_attr_t attr;
+ char name[32];
+ int ret = posix_memalign(&stack_, HEXAGON_STACK_ALIGNMENT,
HEXAGON_STACK_SIZE);
+ CHECK_EQ(ret, 0);
+ qurt_thread_attr_init(&attr);
+ qurt_thread_attr_set_stack_size(&attr, HEXAGON_STACK_SIZE);
+ qurt_thread_attr_set_stack_addr(&attr, stack_);
+ snprintf(name, sizeof(name), "worker %d", id++);
+ qurt_thread_attr_set_name(&attr, name);
+ ret = qurt_thread_create(&thread_, &attr, (void (*)(void*))RunFunction,
this);
+ CHECK_EQ(ret, QURT_EOK);
+ }
+ QuRTThread(QuRTThread&& other) : thread_(other.thread_), f_(other.f_),
stack_(other.stack_) {
+ other.thread_ = 0;
+ }
+ ~QuRTThread() {
+ if (thread_) {
+ join();
+ free(stack_);
+ }
+ }
+ bool joinable() const { return qurt_thread_get_id() != thread_; }
+ void join() {
+ int status;
+ qurt_thread_join(thread_, &status);
+ }
+
+ private:
+ static void RunFunction(QuRTThread* t) {
+ t->f_();
+ qurt_thread_exit(QURT_EOK);
+ }
+ qurt_thread_t thread_;
+ Callback f_;
Review Comment:
use a more meaningful name to replace f_, for example 'worker_callback_'?
##########
src/runtime/threading_backend.cc:
##########
@@ -34,13 +34,63 @@
#endif
#if defined(__hexagon__)
#include <dlfcn.h>
+#include <qurt.h>
+#include <stdlib.h>
+#define HEXAGON_STACK_SIZE 65536
+#define HEXAGON_STACK_ALIGNMENT 32
#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__
+// pthreads are broken on older versions of qurt, so
+// we need to use native APIs instead of std::threads
+class QuRTThread {
+ typedef std::function<void()> Callback;
+
+ public:
+ explicit QuRTThread(Callback worker_callback) : f_(worker_callback) {
Review Comment:
check whether 'worker_callback' is valid
##########
src/runtime/threading_backend.cc:
##########
@@ -34,13 +34,63 @@
#endif
#if defined(__hexagon__)
#include <dlfcn.h>
+#include <qurt.h>
+#include <stdlib.h>
+#define HEXAGON_STACK_SIZE 65536
+#define HEXAGON_STACK_ALIGNMENT 32
#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__
+// pthreads are broken on older versions of qurt, so
+// we need to use native APIs instead of std::threads
+class QuRTThread {
+ typedef std::function<void()> Callback;
+
+ public:
+ explicit QuRTThread(Callback worker_callback) : f_(worker_callback) {
+ static int id = 1;
+ qurt_thread_attr_t attr;
+ char name[32];
+ int ret = posix_memalign(&stack_, HEXAGON_STACK_ALIGNMENT,
HEXAGON_STACK_SIZE);
+ CHECK_EQ(ret, 0);
+ qurt_thread_attr_init(&attr);
+ qurt_thread_attr_set_stack_size(&attr, HEXAGON_STACK_SIZE);
+ qurt_thread_attr_set_stack_addr(&attr, stack_);
+ snprintf(name, sizeof(name), "worker %d", id++);
+ qurt_thread_attr_set_name(&attr, name);
+ ret = qurt_thread_create(&thread_, &attr, (void (*)(void*))RunFunction,
this);
+ CHECK_EQ(ret, QURT_EOK);
+ }
+ QuRTThread(QuRTThread&& other) : thread_(other.thread_), f_(other.f_),
stack_(other.stack_) {
+ other.thread_ = 0;
+ }
+ ~QuRTThread() {
+ if (thread_) {
+ join();
+ free(stack_);
Review Comment:
there is a hide logic that stack_ bind with thread_, but such hide logic is
not guaranteed not change in future, better to independent check and free
them.
if (stack_)free(stack_);
##########
src/runtime/threading_backend.cc:
##########
@@ -34,13 +34,63 @@
#endif
#if defined(__hexagon__)
#include <dlfcn.h>
+#include <qurt.h>
+#include <stdlib.h>
+#define HEXAGON_STACK_SIZE 65536
+#define HEXAGON_STACK_ALIGNMENT 32
#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__
+// pthreads are broken on older versions of qurt, so
+// we need to use native APIs instead of std::threads
+class QuRTThread {
+ typedef std::function<void()> Callback;
+
+ public:
+ explicit QuRTThread(Callback worker_callback) : f_(worker_callback) {
+ static int id = 1;
+ qurt_thread_attr_t attr;
+ char name[32];
+ int ret = posix_memalign(&stack_, HEXAGON_STACK_ALIGNMENT,
HEXAGON_STACK_SIZE);
+ CHECK_EQ(ret, 0);
+ qurt_thread_attr_init(&attr);
+ qurt_thread_attr_set_stack_size(&attr, HEXAGON_STACK_SIZE);
+ qurt_thread_attr_set_stack_addr(&attr, stack_);
+ snprintf(name, sizeof(name), "worker %d", id++);
+ qurt_thread_attr_set_name(&attr, name);
+ ret = qurt_thread_create(&thread_, &attr, (void (*)(void*))RunFunction,
this);
+ CHECK_EQ(ret, QURT_EOK);
+ }
+ QuRTThread(QuRTThread&& other) : thread_(other.thread_), f_(other.f_),
stack_(other.stack_) {
+ other.thread_ = 0;
+ }
+ ~QuRTThread() {
+ if (thread_) {
+ join();
+ free(stack_);
+ }
+ }
+ bool joinable() const { return qurt_thread_get_id() != thread_; }
+ void join() {
+ int status;
+ qurt_thread_join(thread_, &status);
+ }
+
+ private:
+ static void RunFunction(QuRTThread* t) {
Review Comment:
use a more meaningful name for 't', like qrt_thread ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
