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

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

`__kmp_hidden_helper_initialize()` always initializes all hidden threads at once. Right? In this case, you modifications make sense.

I tested the patch applied to the release branch. It fixes both of my reproducers.
Please add my reproducers as test cases.

Also remove the unnecessary code of your first shot.



================
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;
----------------
tianshilei1992 wrote:
> 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.
This change is unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98838