[Openmp-commits] [PATCH] D38185: Implementation of OMPT as specified in OpenMP 5.0 Preview 1

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Some remarks from a high-level look at this revision. Although it might not be possible to split the OMPT changes, any preparation change should go into separate commits.

1. Separate out the changes that are not related to OMPT at all (`.clang-format`, `kmp_task_reduction_nest.cpp`)
2. Deleting the old header files and restricting OMPT to OpenMP 5.0 should be doable in a separate change
3. Are there any changes that are active when OMPT is disabled? We might want to discuss them on their own, all other changes are neatly hidden behind `#define`s

Disclaimer: I worked with Joachim and the others on some of these, so I might be biased that this should go in ;-)



================
Comment at: README.md:1
+# LLVM-openmp branch towards_tr4
+
----------------
This file shouldn't be here


================
Comment at: runtime/.clang-format:5-8
+AlignOperands: false
+DisableFormat:   true
+KeepEmptyLinesAtTheStartOfBlocks: false
+MaxEmptyLinesToKeep: 2
----------------
This should go into its own revision


================
Comment at: runtime/CMakeLists.txt:324-328
+if (NOT (WIN32 OR ${CMAKE_SYSTEM_NAME} MATCHES "Darwin"))
+  # Activate OMPT-SUPPORT by default on Linux machines
+  set(LIBOMP_OMPT_SUPPORT TRUE CACHE BOOL
+    "OMPT-support?")
+else()
----------------
We shouldn't activate this by default until careful testing. And even then, only when building for OpenMP 5.0


================
Comment at: runtime/src/CMakeLists.txt:16-18
+  if(${LIBOMP_OMP_VERSION} LESS 50)
+    message( FATAL_ERROR "OMPT is only available with OpenMP 5.0, LIBOMP_OMP_VERSION is ${LIBOMP_OMP_VERSION}" )
+  endif()
----------------
This should go one level above where all the other checks are performed (`LIBOMP_HAVE_OMPT_SUPPORT`)


================
Comment at: runtime/src/exports_so.txt:120-123
+OMP_4.5 {
+} OMP_4.0;
+OMP_5.0 {
+} OMP_4.5;
----------------
Separate revision. @jlpeyton do we need this?


================
Comment at: runtime/src/ompt-specific.cpp:176-201
+/*ompt_task_info_t *
+__ompt_get_scheduling_taskinfo(int depth)
+{
+    ompt_task_info_t *info = NULL;
+    kmp_info_t *thr = ompt_get_thread();
+
+    if (thr) {
----------------
Please remove and also check that there is no other commented code


================
Comment at: runtime/test/lit.cfg:110-112
+# to run with icc INTEL_LICENSE_FILE must be set
+if 'INTEL_LICENSE_FILE' in os.environ:
+    config.environment['INTEL_LICENSE_FILE'] = os.environ['INTEL_LICENSE_FILE']
----------------
Separate revision


================
Comment at: runtime/test/lit.cfg:118-121
+config.substitutions.append(("%libomp-cxx-compile-and-run", \
+    "%libomp-cxx-compile && %libomp-run"))
+config.substitutions.append(("%libomp-cxx-compile", \
+    "%clangXX %cflags -std=c++11 %s -o %t" + libs))
----------------
AFAICS this isn't related to OMPT?


================
Comment at: runtime/test/lit.cfg:126-128
+config.substitutions.append(("%filecheck-threads", "FileCheck --check-prefixes " + "THREADS"))
+config.substitutions.append(("%filecheck-custom", "FileCheck "))
+config.substitutions.append(("%filecheck", "FileCheck --check-prefixes " + config.filecheck_prefixes))
----------------
No, please keep the prefixes in the tests so that it is clearly visible which prefixes are checked!


https://reviews.llvm.org/D38185