[OpenMP] libomptarget: move debugging dumps under control of environment variable OMP_TARGET_DEBUG

[sdmitriev]

Summary

Libomptarget currently dumps lots of debugging messages to stderr by default (in debug build) and the only way to turn debugging dumps off is rebuilding libomptarget in release mode which is not very convenient.

Attached patch disables default debugging dumps for libomptarget and plugins and moves dumps under control of environment variable OMP_TARGET_DEBUG=<integer>. Dumps are enabled when OMP_TARGET_DEBUG environment variable is set to a positive integer value.

Debugging dumps are available only in debug build; release build does not support it.

[grokos]

Is there a reason why we have a separate int DebugLevel in every plugin source file and omptarget.cpp and parse OMP_TARGET_DEBUG from all those places instead of declaring extern int DebugLevel in omptarget.h, defining int DebugLevel in omptarget.cpp and parsing OMP_TARGET_DEBUG only from within RTLsTy::LoadRTLs()?

[grokos]

I've added Jonas as a reviewer because I'm not very familiar with regression tests.

Jonas, can you review the changes under libomptarget/test?

[sdmitriev]

Defining int DebugLevel in omptarget.cpp only would create a dependency between libomptarget.so and each plugin which I guess should be avoided. I assume we do not want to link plugins with libomptarget library.

[sdmitriev]

Ping

[tlwilmar]

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

[sdmitriev]

I can change the name for this environment variable it as you suggested, but it will make it consistent with other environment variable's name that is already used by libomptarget - OMP_TARGET_OFFLOAD (look at openmp/libomptarget/src/omptarget.cpp, line 285).

[sdmitriev]

*inconsistent

[tlwilmar]

Hmmm... that's disturbing.

The policy for libomp is, if it's in the spec, it starts with OMP_, and if it's just in our implementation, it starts with KMP_.
I guess this is not true for libomptarget. I'd like to get Ravi's opinion on this, so don't make a change yet.

[Hahnfeld]

In D33227#769276, @sdmitriev wrote:

I can change the name for this environment variable it as you suggested, but it will make it consistent with other environment variable's name that is already used by libomptarget - OMP_TARGET_OFFLOAD (look at openmp/libomptarget/src/omptarget.cpp, line 285).

I criticized that when the code went in. The convincing argument here was that OMP_NUM_TEAMS may be defined in a later version of the spec and OMP_TARGET_OFFLOAD was already used for some bots. As that's not yet true for OMP_TARGET_DEBUG, I would also prefer a different name. Ideally, we should also get rid of OMP_TARGET_OFFLOAD and rename it to something different...

[sdmitriev]

I have changed the name of environment variable which controls debugging dumps to LIBOMPTARGET_DEBUG.

[guansong]

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

[grokos]

In D33227#775337, @guansong wrote:

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

[guansong]

In D33227#775415, @grokos wrote:

In D33227#775337, @guansong wrote:

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

No. I am thinking one should be OK for all plugins.

For LIBOMPTARGET_DEBUG, will it control the device side of the debug message later on?

You call it "target", that gives me the impression it will. But maybe that is not the case. And LIBOMPOFFLOAD_DEBUG may be a better name.

In my scenario, I do have two flags, one is for the host (actually the code is almost identical to yours), and one for the device.

The device side of the variable may need to be specific to each device. The one I have can work for GPU, but it may not work well for PHI. As in this case the device and host will share the same openmp runtime mechanism?

[sdmitriev]

In D33227#775572, @guansong wrote:

In D33227#775415, @grokos wrote:

In D33227#775337, @guansong wrote:

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

No. I am thinking one should be OK for all plugins.

For LIBOMPTARGET_DEBUG, will it control the device side of the debug message later on?

Libomptarget itself does not have a device side part yet, but if we decide to add it in future I think it would be consistent to control all messages coming from it by LIBOMPTARGET_DEBUG setting.

You call it "target", that gives me the impression it will. But maybe that is not the case. And LIBOMPOFFLOAD_DEBUG may be a better name.

No, the reason why it is named this way is actually different – it controls debugging messages from the libomptarget library, so it looks quite natural to call it LIBOMPTARGET_DEBUG.

In my scenario, I do have two flags, one is for the host (actually the code is almost identical to yours), and one for the device.

The device side of the variable may need to be specific to each device. The one I have can work for GPU, but it may not work well for PHI. As in this case the device and host will share the same openmp runtime mechanism?

I guess you are talking about the device side RTL which is not directly related to libomptarget and plugins, right? If that is the case then I think we cannot restrict device libraries to use whatever they want. It should be fine to have device specific settings for each particular target.

LIBOMPTARGET_DEBUG is intended to control debugging messages for libomptarget and plugins only. Everything else is outside of its control.

[sdmitriev]

In D33227#769266, @tlwilmar wrote:

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

Terry, I have changed the environment variable name as you suggested. Do you have more comments for this patch?

[tlwilmar]

In D33227#792489, @sdmitriev wrote:

In D33227#769266, @tlwilmar wrote:

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

Terry, I have changed the environment variable name as you suggested. Do you have more comments for this patch?

No more comments. Thanks!

[sdmitriev]

If we reached an agreement on the environment variable name and there are no more comments for this change, can it be approved?

[Hahnfeld]

Given the lack of additional comments, LGTM. Please adapt the commit description to reflect the changes during review.

openmp/trunk/libomptarget/plugins/cuda/src/rtl.cpp

 #include <vector>
 
 #include "omptargetplugin.h"
 
 #ifndef TARGET_NAME
 #define TARGET_NAME CUDA
 #endif
 
+#ifdef OMPTARGET_DEBUG
+static int DebugLevel = 0;
+
 #define GETNAME2(name) #name
 #define GETNAME(name) GETNAME2(name)
-#define DP(...) DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__)
+#define DP(...) \
+  do { \
+    if (DebugLevel > 0) { \
+      DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__); \
+    } \
+  } while (false)
+#else // OMPTARGET_DEBUG
+#define DP(...) {}
+#endif // OMPTARGET_DEBUG
 
 #include "../../common/elf_common.c"
 
 // Utility for retrieving and printing CUDA error string.
 #ifdef CUDA_ERROR_REPORT
 #define CUDA_ERR_STRING(err)                                                   \
   do {                                                                         \
     const char *errStr;                                                        \
void clearOffloadEntriesTable(int32_t device_id) {
     assert(device_id < (int32_t)FuncGblEntries.size() &&
            "Unexpected device id!");
     FuncOrGblEntryTy &E = FuncGblEntries[device_id];
     E.Entries.clear();
     E.Table.EntriesBegin = E.Table.EntriesEnd = 0;
   }
 
   RTLDeviceInfoTy() {
+#ifdef OMPTARGET_DEBUG
+    if (char *envStr = getenv("LIBOMPTARGET_DEBUG")) {
+      DebugLevel = std::stoi(envStr);
+    }
+#endif // OMPTARGET_DEBUG
+
     DP("Start initializing CUDA\n");
 
     CUresult err = cuInit(0);
     if (err != CUDA_SUCCESS) {
       DP("Error when initializing CUDA\n");
       CUDA_ERR_STRING(err);
       return;
     }

openmp/trunk/libomptarget/plugins/generic-elf-64bit/src/rtl.cpp

 #include <cstdio>
 #include <cstring>
 #include <cstdlib>
 #include <dlfcn.h>
 #include <ffi.h>
 #include <gelf.h>
 #include <link.h>
 #include <list>
+#include <string>
 #include <vector>
 
 #include "omptargetplugin.h"
 
 #ifndef TARGET_NAME
 #define TARGET_NAME Generic ELF - 64bit
 #endif
 
 #ifndef TARGET_ELF_ID
 #define TARGET_ELF_ID 0
 #endif
 
+#ifdef OMPTARGET_DEBUG
+static int DebugLevel = 0;
+
 #define GETNAME2(name) #name
 #define GETNAME(name) GETNAME2(name)
-#define DP(...) DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__)
+#define DP(...) \
+  do { \
+    if (DebugLevel > 0) { \
+      DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__); \
+    } \
+  } while (false)
+#else // OMPTARGET_DEBUG
+#define DP(...) {}
+#endif // OMPTARGET_DEBUG
 
 #include "../../common/elf_common.c"
 
 #define NUMBER_OF_DEVICES 4
 #define OFFLOADSECTIONNAME ".omp_offloading.entries"
 
 /// Array of Dynamic libraries loaded for this target.
 struct DynLibTy {
public:
   __tgt_target_table *getOffloadEntriesTable(int32_t device_id) {
     assert(device_id < (int32_t)FuncGblEntries.size() &&
            "Unexpected device id!");
     FuncOrGblEntryTy &E = FuncGblEntries[device_id];
 
     return &E.Table;
   }
 
-  RTLDeviceInfoTy(int32_t num_devices) { FuncGblEntries.resize(num_devices); }
+  RTLDeviceInfoTy(int32_t num_devices) {
+#ifdef OMPTARGET_DEBUG
+    if (char *envStr = getenv("LIBOMPTARGET_DEBUG")) {
+      DebugLevel = std::stoi(envStr);
+    }
+#endif // OMPTARGET_DEBUG
+
+    FuncGblEntries.resize(num_devices);
+  }
 
   ~RTLDeviceInfoTy() {
     // Close dynamic libraries
     for (auto &lib : DynLibs) {
       if (lib.Handle) {
         dlclose(lib.Handle);
         remove(lib.FileName);
       }

openmp/trunk/libomptarget/src/omptarget.cpp

 #include <map>
 #include <mutex>
 #include <string>
 #include <vector>
 
 // Header file global to this project
 #include "omptarget.h"
 
-#define DP(...) DEBUGP("Libomptarget", __VA_ARGS__)
+#ifdef OMPTARGET_DEBUG
+static int DebugLevel = 0;
+
+#define DP(...) \
+  do { \
+    if (DebugLevel > 0) { \
+      DEBUGP("Libomptarget", __VA_ARGS__); \
+    } \
+  } while (false)
+#else // OMPTARGET_DEBUG
+#define DP(...) {}
+#endif // OMPTARGET_DEBUG
+
 #define INF_REF_CNT (LONG_MAX>>1) // leave room for additions/subtractions
 #define CONSIDERED_INF(x) (x > (INF_REF_CNT>>1))
 
 // List of all plugins that can support offloading.
 static const char *RTLNames[] = {
     /* PowerPC target */ "libomptarget.rtl.ppc64.so",
     /* x86_64 target  */ "libomptarget.rtl.x86_64.so",
     /* CUDA target    */ "libomptarget.rtl.cuda.so",
public:
 
   explicit RTLsTy() {}
 
   // Load all the runtime libraries (plugins) if not done before.
   void LoadRTLsOnce();
 };
 
 void RTLsTy::LoadRTLs() {
+#ifdef OMPTARGET_DEBUG
+  if (char *envStr = getenv("LIBOMPTARGET_DEBUG")) {
+    DebugLevel = std::stoi(envStr);
+  }
+#endif // OMPTARGET_DEBUG
+
   // Parse environment variable OMP_TARGET_OFFLOAD (if set)
   char *envStr = getenv("OMP_TARGET_OFFLOAD");
   if (envStr && !strcmp(envStr, "DISABLED")) {
     DP("Target offloading disabled by environment\n");
     return;
   }
 
   DP("Loading RTLs...\n");

openmp/trunk/libomptarget/test/CMakeLists.txt

 # CMakeLists.txt file for unit testing OpenMP Library
 include(FindPythonInterp)
 include(CheckTypeSize)
 if(NOT PYTHONINTERP_FOUND)
   libomptarget_warning_say("Could not find Python.")
   libomptarget_warning_say("The check-libomptarget target will not be available!")
   return()
 endif()
 
 set(LIBOMPTARGET_TEST_CFLAGS "" CACHE STRING
   "Extra compiler flags to send to the test compiler")
 
+if(LIBOMPTARGET_CMAKE_BUILD_TYPE MATCHES debug)
+  set(LIBOMPTARGET_DEBUG True)
+else()
+  set(LIBOMPTARGET_DEBUG False)
+endif()
+
 if(${LIBOMPTARGET_STANDALONE_BUILD})
   # Make sure we can use the console pool for recent cmake and ninja > 1.5
   if(CMAKE_VERSION VERSION_LESS 3.1.20141117)
     set(cmake_3_2_USES_TERMINAL)
   else()
     set(cmake_3_2_USES_TERMINAL USES_TERMINAL)
   endif()
   set(LIBOMPTARGET_TEST_C_COMPILER ${CMAKE_C_COMPILER} CACHE STRING

openmp/trunk/libomptarget/test/env/omp_target_debug.c

+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu && env LIBOMPTARGET_DEBUG=1 %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 | %fcheck-aarch64-unknown-linux-gnu -allow-empty -check-prefix=DEBUG
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu && env LIBOMPTARGET_DEBUG=0 %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 | %fcheck-aarch64-unknown-linux-gnu -allow-empty -check-prefix=NDEBUG
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu && env LIBOMPTARGET_DEBUG=1 %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 | %fcheck-powerpc64-ibm-linux-gnu -allow-empty -check-prefix=DEBUG
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu && env LIBOMPTARGET_DEBUG=0 %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 | %fcheck-powerpc64-ibm-linux-gnu -allow-empty -check-prefix=NDEBUG
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu && env LIBOMPTARGET_DEBUG=1 %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 | %fcheck-powerpc64le-ibm-linux-gnu -allow-empty -check-prefix=DEBUG
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu && env LIBOMPTARGET_DEBUG=0 %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 | %fcheck-powerpc64le-ibm-linux-gnu -allow-empty -check-prefix=NDEBUG
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu && env LIBOMPTARGET_DEBUG=1 %libomptarget-run-x86_64-pc-linux-gnu 2>&1 | %fcheck-x86_64-pc-linux-gnu -allow-empty -check-prefix=DEBUG
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu && env LIBOMPTARGET_DEBUG=0 %libomptarget-run-x86_64-pc-linux-gnu 2>&1 | %fcheck-x86_64-pc-linux-gnu -allow-empty -check-prefix=NDEBUG
+// REQUIRES: libomptarget-debug
+
+int main(void) {
+#pragma omp target
+  {}
+  return 0;
+}
+
+// DEBUG: Libomptarget
+// NDEBUG-NOT: Libomptarget
+// NDEBUG-NOT: Target
+

openmp/trunk/libomptarget/test/lit.cfg

config.test_cflags = config.test_openmp_flag + \
     " -L " + config.library_dir;
 
 if config.omp_host_rtl_directory:
     config.test_cflags = config.test_cflags + " -L " + \
         config.omp_host_rtl_directory
 
 config.test_cflags = config.test_cflags + " " + config.test_extra_cflags
 
+if config.libomptarget_debug:
+  config.available_features.add('libomptarget-debug')
+
 # Setup environment to find dynamic library at runtime
 if config.operating_system == 'Windows':
     append_dynamic_library_path('PATH', config.library_dir, ";")
     append_dynamic_library_path('PATH', config.omp_host_rtl_directory, ";")
 elif config.operating_system == 'Darwin':
     append_dynamic_library_path('DYLD_LIBRARY_PATH', config.library_dir, ":")
     append_dynamic_library_path('DYLD_LIBRARY_PATH', \
         config.omp_host_rtl_directory, ";")
if libomptarget_target in config.libomptarget_system_targets:
             "%libomptarget-compilexx-and-run-" + libomptarget_target + \
             " | " + config.libomptarget_filecheck + " %s"))
         config.substitutions.append(("%libomptarget-compile-run-and-check-" + \
             libomptarget_target, \
             "%libomptarget-compile-and-run-" + libomptarget_target + \
             " | " + config.libomptarget_filecheck + " %s"))
         config.substitutions.append(("%libomptarget-compilexx-and-run-" + \
             libomptarget_target, \
-            "%clangxx-" + libomptarget_target + " %s -o %t-" + \
-            libomptarget_target + " && %t-" + libomptarget_target))
+            "%libomptarget-compilexx-" + libomptarget_target + " && " + \
+            "%libomptarget-run-" + libomptarget_target))
         config.substitutions.append(("%libomptarget-compile-and-run-" + \
             libomptarget_target, \
+            "%libomptarget-compile-" + libomptarget_target + " && " + \
+            "%libomptarget-run-" + libomptarget_target))
+        config.substitutions.append(("%libomptarget-compilexx-" + \
+            libomptarget_target, \
+            "%clangxx-" + libomptarget_target + " %s -o %t-" + \
+            libomptarget_target))
+        config.substitutions.append(("%libomptarget-compile-" + \
+            libomptarget_target, \
             "%clang-" + libomptarget_target + " %s -o %t-" + \
-            libomptarget_target + " && %t-" + libomptarget_target))
+            libomptarget_target))
+        config.substitutions.append(("%libomptarget-run-" + \
+            libomptarget_target, \
+            "%t-" + libomptarget_target))
         config.substitutions.append(("%clangxx-" + libomptarget_target, \
             "%clangxx %cflags -fopenmp-targets=" + libomptarget_target))
         config.substitutions.append(("%clang-" + libomptarget_target, \
             "%clang %cflags -fopenmp-targets=" + libomptarget_target))
         config.substitutions.append(("%fcheck-" + libomptarget_target, \
             config.libomptarget_filecheck + " %s"))
     else:
         config.substitutions.append(("%libomptarget-compile-run-and-check-" + \
             libomptarget_target, \
             "echo ignored-command"))
         config.substitutions.append(("%libomptarget-compilexx-run-and-check-" + \
             libomptarget_target, \
             "echo ignored-command"))
         config.substitutions.append(("%libomptarget-compile-and-run-" + \
             libomptarget_target, \
             "echo ignored-command"))
         config.substitutions.append(("%libomptarget-compilexx-and-run-" + \
             libomptarget_target, \
             "echo ignored-command"))
+        config.substitutions.append(("%libomptarget-compilexx-" + \
+            libomptarget_target, \
+            "echo ignored-command"))
+        config.substitutions.append(("%libomptarget-compile-" + \
+            libomptarget_target, \
+            "echo ignored-command"))
+        config.substitutions.append(("%libomptarget-run-" + \
+            libomptarget_target, \
+            "echo ignored-command"))
         config.substitutions.append(("%clang-" + libomptarget_target, \
             "echo ignored-command"))
         config.substitutions.append(("%clangxx-" + libomptarget_target, \
             "echo ignored-command"))
         config.substitutions.append(("%fcheck-" + libomptarget_target, \
             "echo ignored-command"))
 
 config.substitutions.append(("%clangxx", config.test_cxx_compiler))
 config.substitutions.append(("%clang", config.test_c_compiler))
 config.substitutions.append(("%openmp_flag", config.test_openmp_flag))
 config.substitutions.append(("%cflags", config.test_cflags))

openmp/trunk/libomptarget/test/lit.site.cfg.in

 @AUTO_GEN_COMMENT@
 
 config.test_c_compiler = "@LIBOMPTARGET_TEST_C_COMPILER@"
 config.test_cxx_compiler = "@LIBOMPTARGET_TEST_CXX_COMPILER@"
 config.test_openmp_flag = "@LIBOMPTARGET_TEST_OPENMP_FLAG@"
 # For the moment we still need to pass libomptarget explicitly. Once the driver
 # patch, lands, this is not required anymore.
 config.test_extra_cflags = "-lomptarget @LIBOMPTARGET_TEST_CFLAGS@"
 config.libomptarget_obj_root = "@CMAKE_CURRENT_BINARY_DIR@"
 config.library_dir = "@LIBOMPTARGET_LIBRARY_DIR@"
 config.omp_header_directory = "@LIBOMPTARGET_OPENMP_HEADER_FOLDER@"
 config.omp_host_rtl_directory = "@LIBOMPTARGET_OPENMP_HOST_RTL_FOLDER@"
 config.operating_system = "@CMAKE_SYSTEM_NAME@"
 config.libomptarget_all_targets = "@LIBOMPTARGET_ALL_TARGETS@".split()
 config.libomptarget_system_targets = "@LIBOMPTARGET_SYSTEM_TARGETS@".split()
 config.libomptarget_filecheck = "@LIBOMPTARGET_FILECHECK_EXECUTABLE@"
+config.libomptarget_debug = @LIBOMPTARGET_DEBUG@
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LIBOMPTARGET_BASE_DIR@/test/lit.cfg")