[GitHub] [tvm] JosephTheOctonaut commented on a diff in pull request #12905: [Hexagon] [runtime] Add thread manager to resource management

[GitBox <gi...@apache.org>]

JosephTheOctonaut commented on code in PR #12905:
URL: https://github.com/apache/tvm/pull/12905#discussion_r980583486


##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -164,6 +176,12 @@ class HexagonDeviceAPI final : public DeviceAPI {
 
   //! \brief Current buffer manager
   HexagonBufferManager* mgr;
+
+  //! \brief Thread manager
+  std::unique_ptr<HexagonThreadManager> runtime_threads;
+  const unsigned threads{6};

Review Comment:
   `pipe_size` can probably be increased. This number will determine the max depth of the dispatch queue for each thread, which might fill up with size 100. E.g., if you dispatch all the compute calls for a kernel before starting the threads, it might come out to >100 dispatches for a given thread if the kernel is big enough.
   
   I don't think there's a negative associated with increasing it, so maybe 1,000 words?



##########
src/runtime/hexagon/hexagon_device_api.h:
##########
@@ -54,17 +55,26 @@ class HexagonDeviceAPI final : public DeviceAPI {
   void AcquireResources() {
     CHECK_EQ(runtime_hexbuffs, nullptr);
     runtime_hexbuffs = std::make_unique<HexagonBufferManager>();
-    LOG(INFO) << "runtime_hexbuffs created";
+    DLOG(INFO) << "runtime_hexbuffs created";
     mgr = runtime_hexbuffs.get();
+
+    CHECK_EQ(runtime_threads, nullptr);
+    runtime_threads = std::make_unique<HexagonThreadManager>(threads, stack_size, pipe_size);
+    DLOG(INFO) << "runtime_threads created";
   }
 
   //! \brief Ensures all runtime resources are freed
   void ReleaseResources() {
+    CHECK(runtime_threads) << "runtime_threads was not created in AcquireResources";
+    runtime_threads.reset();

Review Comment:
   Just to be clear here (since it might not be apparent from the thread manager interface): calling the destructor of the `HexagonThreadManager` will block until all of its threads have finished working. If work has been dispatched but the threads haven't been started yet, the destructor will start the threads and wait for them to finish.
   
   I'm not sure on the use cases we want to cover here, but there is no "graceful crash" functionality right now, in case you wanted `ReleaseResources()` to be capable of terminating running (or deadlocked) threads.



-- 
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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org