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

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added inline comments.


================
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;
----------------
protze.joachim wrote:
> 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.
Even w/o hidden helper thread, expansion by 1 will not result in increment by 1 because the `newCapacity` always doubles. Say originally it is 32, and we ask for expansion by 1, `newCapacity` will be 64 instead of 33. Therefore, whether we add extra space for hidden helper thread doesn’t waste too much memory here.


================
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;
----------------
protze.joachim wrote:
> Please don't forget about this fix. I don't care whether you fix it here or in a follow-up patch.
Sure. I’ll include in this 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++)
         ;
----------------
protze.joachim wrote:
> 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.
> 
Because we already take the number of hidden helper thread into account when setting `__kmp_threads_capacity`, this assumption holds, right?


================
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()),
----------------
protze.joachim wrote:
> `omp_get_num_threads()` will always return 1 in serial context.
This logic is from code to set the capacity, specifically in the comment. It uses `$OMP_NUM_THREADS` originally so here I changed to `omp_get_num_threads()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98838