[OpenMP] Disable hidden helper task by default

[tianshilei1992]

Summary

Performance regression is reported in D98838 when enabling hidden helper thread. This patch disables it by default.

[ronlieb]

Thanks for putting up this change, just a few comments.

openmp/runtime/src/kmp_runtime.cpp

8648

i tried this patch and found that i still needed to set

LIBOMP_NUM_HIDDEN_HELPER_THREADS=0

would it make sense to set the default to 0 instead of 8?

openmp/runtime/src/kmp_settings.cpp

1224

is this function called only when the environment variable is explicitly set in the environment?

[tianshilei1992]

openmp/runtime/src/kmp_runtime.cpp

8648

Yeah, that's a nice catch. It is this "hole" in __kmp_threads affecting the performance from my wild guess. I'll set it to 0 for now and fix it later. Thanks.

openmp/runtime/src/kmp_settings.cpp

1224

You're right.

[tianshilei1992]

comments

[tianshilei1992]

openmp/runtime/src/kmp_settings.cpp

1227

This is kind of a temporary solution here. If LIBOMP_USE_HIDDEN_HELPER_TASK is set, __kmp_hidden_helper_threads_num is set to 8.

[jdoerfert]

@ronlieb, looks good?

[ronlieb]

[AMD Official Use Only - Internal Distribution Only]

Testing it now

Get Outlook for iOShttps://aka.ms/o0ukef

---

From: Johannes Doerfert via Phabricator <[e-mail]>
Sent: Sunday, March 21, 2021 12:53:31 PM
To: [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Cc: Lieberman, Ron <[e-mail]>; Liu, Yaxun (Sam) <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Kumar N, Bhuvanendra <[e-mail]>; [e-mail] <[e-mail]>; Singh, Pushpinder <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Islam, Saiyedul <[e-mail]>; Tomar, Sourabh Singh <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Subject: [PATCH] D99020: [OpenMP] Disable hidden helper task by default

[CAUTION: External Email]

jdoerfert added a comment.

@ronlib, looks good?

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD99020%2Fnew%2F&data=04%7C01%7Cron.lieberman%40amd.com%7C3ad37352fe4a4eedad0a08d8ec89dc7d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637519424152202471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YBEUHHc%2BUmjwT80NieUoLjo%2FQAJoLWMLx16npDTy9OI%3D&reserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD99020&amp;data=04%7C01%7Cron.lieberman%40amd.com%7C3ad37352fe4a4eedad0a08d8ec89dc7d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637519424152202471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=e%2FFTbFgb2gKOGMuSyHeaRKSelewsbGIBbZIooFI2wrg%3D&amp;reserved=0

[ronlieb]

619.lbm looks good base and peak.
Testing all of fpspeed base, should have result in an hour or two.
but i think it is good.

[ronlieb]

fpspeed base - looks good.

[tianshilei1992]

update tests

[ronlieb]

planning to land this ?

[tianshilei1992]

In D99020#2644978, @ronlieb wrote:

planning to land this ?

It will not be landed because it's not for trunk. I've filed a ticket to request the release manager to pick the patch to the release branch.
https://bugs.llvm.org/show_bug.cgi?id=49673 FYI

[tianshilei1992]

This patch is useless as the performance issue was fixed.

openmp/runtime/src/kmp_runtime.cpp

 /*
  * kmp_runtime.cpp -- KPTS runtime support library
  */
 
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
if (__kmp_init_serial == 0)
     __kmp_do_serial_initialize();
   __kmp_display_env_impl(!verbose, verbose);
   __kmp_release_bootstrap_lock(&__kmp_initz_lock);
 }
 
 // Globals and functions for hidden helper task
 kmp_info_t **__kmp_hidden_helper_threads;
 kmp_info_t *__kmp_hidden_helper_main_thread;
-kmp_int32 __kmp_hidden_helper_threads_num = 8;
+kmp_int32 __kmp_hidden_helper_threads_num = 0;
 std::atomic<kmp_int32> __kmp_unexecuted_hidden_helper_tasks;
-#if KMP_OS_LINUX
-kmp_int32 __kmp_enable_hidden_helper = TRUE;
-#else
 kmp_int32 __kmp_enable_hidden_helper = FALSE;
-#endif
 
 namespace {
 std::atomic<kmp_int32> __kmp_hit_hidden_helper_threads_num;
 
 void __kmp_hidden_helper_wrapper_fn(int *gtid, int *, ...) {
   // This is an explicit synchronization on all hidden helper threads in case
   // that when a regular thread pushes a hidden helper task to one hidden
   // helper thread, the thread has not been awaken once since they're released

openmp/runtime/src/kmp_settings.cpp

 /*
  * kmp_settings.cpp -- Initialize environment variables
  */
 
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
static void __kmp_stg_parse_use_hidden_helper(char const *name,
                                               char const *value, void *data) {
   __kmp_stg_parse_bool(name, value, &__kmp_enable_hidden_helper);
 #if !KMP_OS_LINUX
   __kmp_enable_hidden_helper = FALSE;
   K_DIAG(1,
          ("__kmp_stg_parse_use_hidden_helper: Disable hidden helper task on "
           "non-Linux platform although it is enabled by user explicitly.\n"));
 #endif
+  // Set the number to 0 if hidden helper task is disabled
+  if (__kmp_enable_hidden_helper == FALSE) {
+    __kmp_hidden_helper_threads_num = 0;
+  } else {
+    __kmp_hidden_helper_threads_num = 8;
+  }
 } // __kmp_stg_parse_use_hidden_helper
 
 static void __kmp_stg_print_use_hidden_helper(kmp_str_buf_t *buffer,
                                               char const *name, void *data) {
   __kmp_stg_print_bool(buffer, name, __kmp_enable_hidden_helper);
 } // __kmp_stg_print_use_hidden_helper
 
 static void __kmp_stg_print_num_threads(kmp_str_buf_t *buffer, char const *name,

openmp/runtime/test/tasking/hidden_helper_task/gtid.cpp

 // RUN: %libomp-cxx-compile-and-run
+// XFAIL: *
 
 /*
  * This test aims to check whether hidden helper thread has right gtid. We also
  * test if there is mixed dependences between regular tasks and hidden helper
  * tasks, the tasks are executed by right set of threads. It is equivalent to
  * the following code:
  *
  * #pragma omp parallel for

openmp/runtime/test/worksharing/for/kmp_sch_simd_guided.c

 extern void __kmpc_barrier(id*, int gtid);
 extern void __kmpc_dispatch_init_4(id*, int, enum sched, int, int, int, int);
 extern void __kmpc_dispatch_init_8(id*, int, enum sched, i64, i64, i64, i64);
 extern int __kmpc_dispatch_next_4(id*, int, void*, void*, void*, void*);
 extern int __kmpc_dispatch_next_8(id*, int, void*, void*, void*, void*);
 // End of definitions copied from OpenMP RTL.
 // ---------------------------------------------------------------------------
 static id loc = {0, 2, 0, 0, ";file;func;0;0;;"};
-// This variable is defined in OpenMP RTL but we can't have it exposed so we
-// need to redefine it here.
-static int __kmp_hidden_helper_threads_num = 8;
 
 // ---------------------------------------------------------------------------
 int run_loop_64(i64 loop_lb, i64 loop_ub, i64 loop_st, int loop_chunk) {
   int err = 0;
   static int volatile loop_sync = 0;
   i64 lb;   // Chunk lower bound
   i64 ub;   // Chunk upper bound
   i64 st;   // Chunk stride
   int rc;
   int tid = omp_get_thread_num();
   int gtid = tid;
-  if (gtid) {
-    gtid += __kmp_hidden_helper_threads_num;
-  }
   int last;
 #if DEBUG
   printf("run_loop_<%d>(lb=%d, ub=%d, st=%d, ch=%d)\n",
     (int)sizeof(i64), gtid, tid,
     (int)loop_lb, (int)loop_ub, (int)loop_st, loop_chunk);
 #endif
   // Don't test degenerate cases that should have been discovered by codegen
   if (loop_st == 0)
int run_loop_32(int loop_lb, int loop_ub, int loop_st, int loop_chunk) {
   int err = 0;
   static int volatile loop_sync = 0;
   int lb;   // Chunk lower bound
   int ub;   // Chunk upper bound
   int st;   // Chunk stride
   int rc;
   int tid = omp_get_thread_num();
   int gtid = tid;
-  if (gtid) {
-    gtid += __kmp_hidden_helper_threads_num;
-  }
   int last;
 #if DEBUG
   printf("run_loop_<%d>(lb=%d, ub=%d, st=%d, ch=%d)\n",
     (int)sizeof(int), gtid, tid,
     (int)loop_lb, (int)loop_ub, (int)loop_st, loop_chunk);
 #endif
   // Don't test degenerate cases that should have been discovered by codegen
   if (loop_st == 0)
#pragma omp parallel num_threads(num_th)
   }; // for chunk
  }
  return err;
 } // run_all
 
 // ---------------------------------------------------------------------------
 int main()
 {
-  {
-    const char *env = getenv("LIBOMP_NUM_HIDDEN_HELPER_THREADS");
-    if (env) {
-      __kmp_hidden_helper_threads_num = atoi(env);
-    }
-  }
-
   int n, err = 0;
   for (n = 1; n <= 4; ++ n) {
     err += run_32(n);
     err += run_64(n);
   }; // for n
   if (err)
     printf("failed with %d errors\n", err);
   else
     printf("passed\n");
   return err;
 }