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

Jonas Hahnfeld via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Mon Sep 25 07:05:52 PDT 2017

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
+  # Activate OMPT-SUPPORT by default on Linux machines
+    "OMPT-support?")
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
+    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!


More information about the Openmp-commits mailing list