[Openmp-commits] [PATCH] D45890: [OMPT] Add implementation and tests of Archer tool

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

The code itself looks good to me (it should probably be `clang-format`ed). If others have comments, please raise them now.

Apart from that, I still found many unused lines in the testing framework...



================
Comment at: tools/archer/README.md:168-177
+In case we installed Archer with the official LLVM OpenMP runtime and
+ThreadSanitizer support, we compile the program as follow:
+
+    clang-archer myprogram.c -o myprogram
+
+otherwise, if we installed Archer with the LLVM OpenMP runtime and
+ThreadSanitizer OMPT support our compile command will look like:
----------------
Do we still want to speak of `clang-archer`?


================
Comment at: tools/archer/tests/CMakeLists.txt:18-19
+set(LIBARCHER_TEST_PATH ${CMAKE_CURRENT_SOURCE_DIR})
+set(LIBARCHER_HAVE_ARCHER_LIBRARY ${LIBARCHER_LIB_PATH})
+set(LIBARCHER_HAVE_ARCHER_RUNTIME ${LIBARCHER_RUNTIME_PATH})
+
----------------
These variables are not defined, so I guess they can be safely removed


================
Comment at: tools/archer/tests/CMakeLists.txt:29-30
+
+pythonize_bool(LIBARCHER_HAVE_ARCHER_LIBRARY)
+pythonize_bool(LIBARCHER_HAVE_ARCHER_RUNTIME)
+pythonize_bool(LIBARCHER_HAVE_LIBATOMIC)
----------------
Likewise


================
Comment at: tools/archer/tests/lit.cfg:53-62
+libs_archer = ""
+if config.has_archer_runtime:
+    libs_archer += " -L" + config.archer_runtime_dir + " -l" + \
+    config.archer_runtime.replace("lib", "").replace(".so", "").replace(".dy", "") + \
+    " -Wl,-rpath," + config.archer_runtime_dir
+
+# if config.has_archer_library:
----------------
I think these lines can be removed, see comments in `CMakeLists.txt` and implications


================
Comment at: tools/archer/tests/lit.cfg:92-96
+config.suppression = ""
+# need to deal with the static analysis with another execution line in the test
+# if not config.has_archer_library and config.has_archer_runtime:
+if config.has_archer_runtime:
+	config.suppression = "env TSAN_OPTIONS=\"ignore_noninstrumented_modules=1\""
----------------
This can probably be removed


================
Comment at: tools/archer/tests/lit.site.cfg.in:13-14
+config.operating_system = "@CMAKE_SYSTEM_NAME@"
+config.hwloc_library_dir = "@LIBOMP_HWLOC_LIBRARY_DIR@"
+config.using_hwloc = @LIBOMP_USE_HWLOC@
+config.has_libm = @LIBOMP_HAVE_LIBM@
----------------
Hahnfeld wrote:
> Still not needed
Ping, there's no definition of `OPENMP_TEST_ARCHER_FLAGS` and no use of `test_archer_flags`


================
Comment at: tools/archer/tests/lit.site.cfg.in:18-24
+config.archer_tools_dir = "@LIBARCHER_TOOLS_DIR@"
+config.archer_library_dir = "@LIBARCHER_LIB_PATH@"
+config.archer_runtime_dir = "@LIBARCHER_RUNTIME_PATH@"
+config.archer_library = "@LIBARCHER_LIB@"
+config.archer_runtime = "@LIBARCHER_RTL@"
+config.has_archer_library = @LIBARCHER_HAVE_ARCHER_LIBRARY@
+config.has_archer_runtime = @LIBARCHER_HAVE_ARCHER_RUNTIME@
----------------
All of these are either not used or are probably not needed, see comment in `CMakeLists.txt`.


================
Comment at: tools/archer/tests/ompt/ompt-signal.h:2
+/*
+ * ompt-signal.h -- Header providing low-level synchronization for tests
+ */
----------------
We should probably work on deduplicating this from the OMPT tests. Not a strict requirement for now, the file is probably small enough...


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

https://reviews.llvm.org/D45890