[Hexagon] Host API for changing stack size by aankit-ca · Pull Request #5708 · halide/Halide

aankit-ca · 2021-02-05T01:59:37Z

@dsharletg @pranavb-ca Users will have to call halide_hexagon_set_thread_params with thread priority and stack size before any RPC call.

dsharletg · 2021-02-05T03:06:41Z

src/runtime/hexagon_remote/host_shim.h

+ * @param datalen, length of data
+ * @retval, 0 on success
+ */
+extern "C" __QAIC_REMOTE_EXPORT int __QAIC_REMOTE(remote_session_control)(uint32_t req, void *data, uint32_t datalen) __QAIC_REMOTE_ATTRIBUTE;


Isn't this code just resolving to the definition in libadsprpc_shim.cpp? In that case, I think most of the stuff in this header is unnecessary (and may be wrong, e.g. dllexport stuff), we can just make a libadsprpc_shim.h and declare it there.

dsharletg · 2021-02-05T08:09:31Z

src/runtime/hexagon_host.cpp

+
+    debug(user_context) << "halide_hexagon_set_thread_params\n";
+    if (!remote_set_thread_params) {
+        // This runtime doesn't support changing the performance target.


performance target -> thread params?

dsharletg · 2021-02-05T08:11:50Z

src/runtime/hexagon_remote/host_shim.cpp

+        __android_log_print(ANDROID_LOG_ERROR, "halide", "Error: Could not set stack_size to %d. remote_session_control returned %d.",
+                            stack_size, retval);
+    }
+    return retval;


I think maybe this should return either halide_error_code_success or halide_error_code_generic_error, I think the current code is returning a Qualcomm error code as something that will probably be interpreted as a Halide error code. The other remote runtime functions are different because we implement them (that said, I've seen qualcomm error codes returned in crash conditions, so this is a bit janky for the other functions too).

I've moved this code to hexagon_host.cpp.

dsharletg · 2021-02-05T08:13:44Z

src/runtime/HalideRuntimeHexagonHost.h

 // This is deprecated.
 typedef halide_hexagon_power_t halide_hvx_power_perf_t;

+/** Set fastRPC thread priority and staksize. Set thread params before making


stack size (in bytes?)

Yes. Updated the comment

dsharletg · 2021-02-05T08:15:42Z

src/runtime/HalideRuntimeHexagonHost.h


+/** Set fastRPC thread priority and staksize. Set thread params before making
+ * any RPC calls. halide_hexagon_set_thread_params. */
+extern int halide_hexagon_set_thread_params(void *user_context, int priority, int stack_size);


Maybe this and halide_hexagon_set_thread_priority should be documented together so they can share the more detailed comment about priorities?

I don't know if halide_hexagon_set_thread_priority mentions that lower int values mean higher priority. We should mention it in the comments here regardless.

halide_hexagon_set_thread_priority does mention about this: "Smaller number for higher priority"

dsharletg · 2021-02-05T08:16:28Z

src/runtime/HalideRuntimeHexagonHost.h

 typedef halide_hexagon_power_t halide_hvx_power_perf_t;

+/** Set fastRPC thread priority and staksize. Set thread params before making
+ * any RPC calls. halide_hexagon_set_thread_params. */


Remove stray function name here?

s/staksize/stack size/

pranavb-ca · 2021-02-05T22:41:22Z

src/runtime/HalideRuntimeHexagonHost.h


+/** Set fastRPC thread priority and staksize. Set thread params before making
+ * any RPC calls. halide_hexagon_set_thread_params. */
+extern int halide_hexagon_set_thread_params(void *user_context, int priority, int stack_size);


I don't know if halide_hexagon_set_thread_priority mentions that lower int values mean higher priority. We should mention it in the comments here regardless.

pranavb-ca · 2021-02-05T22:42:28Z

src/runtime/hexagon_host.cpp

    halide_hexagon_power_hvx_off(user_context);
 }

+WEAK int halide_hexagon_set_thread_params(void *user_context, int priority, int stack_size) {


As discussed offline just now, renaming this to halide_hexagon_set_remote_thread_params makes more sense?

Done. Thanks

dsharletg · 2021-02-06T00:32:59Z

src/runtime/HalideRuntimeHexagonHost.h

 */

 #define HALIDE_RUNTIME_HEXAGON
+#define FASTRPC_THREAD_PARAMS (1)


This (and remote_rpc_thread_params) shouldn't be in this header, they aren't used in the Halide API. I think they should be in hexagon_host.cpp

Done. Also I noticed somewhere the default priority value for fastrpc threads was 192 and not 100. I'll check with @pranavb-ca once.

dsharletg · 2021-02-06T00:35:13Z

src/runtime/HalideRuntimeHexagonHost.h

+    int stack_size;  // user thread stack size, pass -1 to use default
+};
+
+/** Set the default priority/stack_size for Halide Hexagon user threads:


Does set_remote_thread_params apply to all FastRPC threads, or just the calling thread? At first I thought it was the latter, but now the docs make it sound like the former...

I'm not sure about this. Will confirm with Pranav and get back to you.

It will apply to all fastrpc threads spawned by the process and not just he calling thread.

pranavb-ca · 2021-02-08T23:42:26Z

src/runtime/HalideRuntimeHexagonHost.h

 *   - Priority 0 is reserved for OS usage
- * If this routine is not called, the priority will default to 100.
- * This is intended to be called before dispatching any pipeline. */
+ * If halide_hexagon_set_thread_priority routine is not called, the priority


@aankit-ca - With PipelineContext removed halide_remote.cpp, I don't think the plumbing for halide_hexagon_set_thread_priority to get the priority set for each thread in the parallel thread pool exists any more. We should either completely remove this function or change it to use remote_session_control where available and fall back to its current approach now.(I don't know if many people use it though). We should also use the new thread_params mechanism to set the thread priority and/ or stack size.

// to set only the priority halide_hexagon_set_remote_thread_params(NULL, 10, -1); // to set only the stack size halide_hexagon_set_remote_thread_params(NULL, -1, 1024 * 32); // to set both halide_hexagon_set_remote_thread_params(NULL, 10, 1024 * 32);

pranavb-ca · 2021-02-08T23:53:28Z

src/runtime/hexagon_host.cpp

+    debug(user_context) << "        " << result << "\n";
+    if (result != 0) {
+        error(user_context) << "set_remote_thread_params faiiled.\n";
+        return result;


Will this fastrcp error code not get conflated with a halide runtime error code?

Yes, it will, this should return -1 or some other halide error code.

steven-johnson · 2021-02-12T17:51:24Z

Looks like this PR still needs some cleanup before it can land?

steven-johnson · 2021-02-17T18:55:16Z

Where does this PR stand?

aankit-ca · 2021-02-17T18:59:12Z

@steven-johnson The PR needs some changes. Unfortunately my dev machine is down due to power outage since last week. I'll update the PR soon.

pranavb-ca · 2021-02-23T16:42:00Z

@steven-johnson & @dsharletg - Due to the winter storm that Texas saw last week, our office where all our hardware for building and testing is saw a massive power outage. As if that wasn't enough, a sprinkler pipe burst caused flooding in our office. As such we are scrambling to bring up some testing infrastructure for our group. @aankit-ca will post an update on this PR once we have some capability restored.

steven-johnson · 2021-02-23T17:28:22Z

Thanks for the update!

steven-johnson · 2021-06-08T16:54:27Z

This PR has been sitting around for a few months now; is it still active?

aankit-ca · 2021-09-15T18:27:53Z

@steven-johnson Sorry this PR fell through the cracks. But I think we need this PR. I'll resume working on this.

steven-johnson · 2021-09-15T18:30:23Z

@steven-johnson Sorry this PR fell through the cracks. But I think we need this PR. I'll resume working on this.

Thanks for the update!

dsharletg · 2021-09-20T03:10:50Z

We now have another stack usage regression causing crashes. Is this PR something that is ready to merge early this week?

dsharletg · 2021-09-20T03:12:15Z

src/runtime/hexagon_host.cpp

+    debug(user_context) << "halide_hexagon_set_remote_thread_params\n";
+    if (!set_remote_thread_params) {
+        // This runtime doesn't support changing the thread params.
+        return -1;


In other similar "optional" cases where the runtime function is missing, we return 0.

dsharletg · 2021-09-20T03:12:42Z

src/runtime/hexagon_host.cpp

+    debug(user_context) << "        " << result << "\n";
+    if (result != 0) {
+        error(user_context) << "set_remote_thread_params faiiled.\n";
+        return result;


Yes, it will, this should return -1 or some other halide error code.

aankit-ca · 2021-09-28T15:51:59Z

We now have another stack usage regression causing crashes. Is this PR something that is ready to merge early this week?

Saw this late. I've most of the changes ready. Will put them up for review soon.

aankit-ca · 2021-10-06T19:04:33Z

@dsharletg @pranavb-ca PTAL

aankit-ca · 2021-10-14T20:21:48Z

@dsharletg @pranavb-ca Ping

connorsmacd · 2021-10-16T19:00:39Z

src/runtime/hexagon_host.cpp

 WEAK remote_power_mode_fn remote_set_performance_mode = nullptr;
-WEAK remote_thread_priority_fn remote_set_thread_priority = nullptr;
+WEAK remote_set_thread_params_fn remote_set_thread_params = nullptr;
+WEAK remote_session_control_fn remote_thread_session_control = nullptr;


Should thread be included in the name here? According to the Hexagon SDK docs,remote_session_control can be used for more than just setting thread parameters. We may want to reuse this function for setting other non-thread parameters in the future.

In fact, I've been playing around with using "unsigned" execution mode (discussed here: #5200 (comment)). Using this branch, I was able to reuse remote_thread_session_control to enable said mode.

alexreinking · 2026-02-14T15:37:04Z

@aankit-ca - is there any path forward to merging this functionality? How were the binary files generated? In a manner equivalent to #7741?

Add option to set_stack_size

fc68f82

aankit-ca changed the title ~~[hexagon] Host API for setting stack size~~ [Hexagon] Host API for changing stack size Feb 5, 2021

Change stack-size for hexagon-benchmarks

f6fc346

dsharletg reviewed Feb 5, 2021

View reviewed changes

Correction for remote_session_control declaration

c20231a

dsharletg reviewed Feb 5, 2021

View reviewed changes

clang-format

ccacf94

steven-johnson requested a review from halidebuildbots February 5, 2021 17:38

pranavb-ca reviewed Feb 5, 2021

View reviewed changes

aankit-quic added 2 commits February 5, 2021 18:27

Address comments

05f85c6

clang-format

72b31a2

dsharletg reviewed Feb 6, 2021

View reviewed changes

Corrections

5600e3c

pranavb-ca reviewed Feb 8, 2021

View reviewed changes

alexreinking added this to the v12.0.0 milestone Feb 13, 2021

alexreinking modified the milestones: v12.0.0, v13.0.0 Apr 16, 2021

dsharletg reviewed Sep 20, 2021

View reviewed changes

Merge branch 'master' into aankit/stack_size

a69b468

Change stack-size and priority for dsp threads

b1fc7e1

aankit-ca force-pushed the aankit/stack_size branch from 43f3969 to b1fc7e1 Compare September 29, 2021 07:26

minor change

b85f766

connorsmacd reviewed Oct 16, 2021

View reviewed changes

Conversation

aankit-ca commented Feb 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steven-johnson commented Feb 12, 2021

Uh oh!

steven-johnson commented Feb 17, 2021

Uh oh!

aankit-ca commented Feb 17, 2021

Uh oh!

pranavb-ca commented Feb 23, 2021

Uh oh!

steven-johnson commented Feb 23, 2021

Uh oh!

steven-johnson commented Jun 8, 2021

Uh oh!

aankit-ca commented Sep 15, 2021

Uh oh!

steven-johnson commented Sep 15, 2021

Uh oh!

dsharletg commented Sep 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aankit-ca commented Sep 28, 2021

Uh oh!

aankit-ca commented Oct 6, 2021