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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Some high-level comments inline



================
Comment at: runtime/src/ompt-general.cpp:233-235
+#if KMP_OS_UNIX
+  { // Non-standard: load archer tool if application is built with TSan
+#if KMP_OS_UNIX
----------------
These preprocessor guards are redundant.


================
Comment at: runtime/src/ompt-general.cpp:240-245
+#elif 0 && KMP_OS_WINDOWS // Not yet tested. How is the fname of libarcher on Windows?
+    char *fname = "archer.dll";
+    HMODULE h = LoadLibrary(fname);
+    if (h) {
+      start_tool = (ompt_start_tool_t)GetProcAddress(h, "ompt_start_tool");
+#endif
----------------
If it's not tested, it should be removed.


================
Comment at: tools/CMakeLists.txt:7
+  if(IS_DIRECTORY ${entry} AND EXISTS ${entry}/CMakeLists.txt)
+    if((NOT ${entry} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR}/dummy))
+      add_subdirectory(${entry})
----------------
Why do we need to handle this `dummy`?


================
Comment at: tools/archer/CMakeLists.txt:1-4
+#
+# ompt-tsan.cpp -- Archer runtime library, TSan annotations for Archer
+#
+#
----------------
I don't think this should be here...


================
Comment at: tools/archer/LICENSE.txt:1-3
+Apache License
+Version 2.0, January 2004
+http://www.apache.org/licenses/
----------------
there is a LICENSE.txt in the `openmp` folder, I don't think we should have a copy in this subdirectory.


================
Comment at: tools/archer/counter.h:2
+/*
+ * counter.cpp -- Archer runtime library, counter header file
+ */
----------------
counter.h


================
Comment at: tools/archer/ftsan.c:2
+/*
+ * ftsan.c -- Archer runtime library, TSan annotations for Fortran
+ */
----------------
Why do we need this in here? This should surely be handled in the compiler?


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

https://reviews.llvm.org/D45890