[Openmp-commits] [PATCH] D98838: [OpenMP] Fixed a crash in hidden helper thread

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim requested changes to this revision.
protze.joachim added a comment.
This revision now requires changes to proceed.

The test case should really try to exceed the current capacity by 1.

I don't think, this patch really solves the fundamental issue resulting in the assertion.



================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3550-3558
+  // If hidden helper thread is enabled, we also need to count the number
+  if (__kmp_enable_hidden_helper) {
+    if (UNLIKELY(newCapacity >
+                 __kmp_sys_max_nth - __kmp_hidden_helper_threads_num)) {
+      newCapacity = __kmp_sys_max_nth;
+    } else {
+      newCapacity += __kmp_hidden_helper_threads_num;
----------------
I don't think, this is the right fix for the problem. 
`__kmp_threads_capacity` is the size of the `__kmp_threads` array. If a call to `__kmp_expand_threads` asks to expand the array by 1, you don't need to expand by additional hidden threads (as they are not placed at the end). The hidden threads were already part of the `__kmp_threads_capacity` before expansion.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3620
   /* see if there are too many threads */
   if (__kmp_all_nth >= capacity && !__kmp_expand_threads(1)) {
     if (__kmp_tp_cached) {
----------------
This might not call `__kmp_expand_threads(1)`, if not all `__kmp_hidden_helper_threads_num` are created before this code is reached and `__kmp_all_nth - __kmp_created_hidden_helper_threads + __kmp_hidden_helper_threads_num >= capacity`. 


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3651
     // initial thread. Slots for hidden helper threads should also be skipped.
     if (initial_thread && __kmp_threads[0] == NULL) {
       gtid = 0;
----------------
Please don't forget about this fix. I don't care whether you fix it here or in a follow-up patch.


================
Comment at: openmp/runtime/src/kmp_runtime.cpp:3654-3656
       for (gtid = __kmp_hidden_helper_threads_num + 1;
            TCR_PTR(__kmp_threads[gtid]) != NULL; gtid++)
         ;
----------------
This for-loop implicitly assumes, that it will find an empty space in `[0:__kmp_threads_capacity)`, i.e. implicitly assumes `gtid < __kmp_threads_capacity` , as stated in the assertion below - but you skip the first few index in the range. Line 3620 fails to provide this guarantee for your modified numbering scheme.



================
Comment at: openmp/runtime/test/tasking/hidden_helper_task/num_threads.cpp:12
+  const int __kmp_threads_capacity =
+      std::min(std::max(std::max(32, 4 * omp_get_num_threads()),
+                        4 * omp_get_num_procs()),
----------------
`omp_get_num_threads()` will always return 1 in serial context.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98838/new/

https://reviews.llvm.org/D98838