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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

More comments



================
Comment at: tools/CMakeLists.txt:2
+# Discover the tools that use CMake in the subdirectories.
+# Note that explicit cmake invocation is required every time a new project
+# is added or removed.
----------------
s/project/tool/


================
Comment at: tools/archer/CMakeLists.txt:4
+# // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# // See tools/archer/LICENSE.txt for details.
+# // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
----------------
I think this should now say `See https://llvm.org/LICENSE.txt for license information.`, at least that's what other files have.


================
Comment at: tools/archer/CMakeLists.txt:17-18
+install(TARGETS archer archer_static
+  LIBRARY DESTINATION lib
+  ARCHIVE DESTINATION lib)
+
----------------
Please use `OPENMP_INSTALL_LIBDIR` to handle libdir suffix.


================
Comment at: tools/archer/ftsan.c:2
+/*
+ * ftsan.c -- Archer runtime library, TSan annotations for Fortran
+ */
----------------
protze.joachim wrote:
> Hahnfeld wrote:
> > Why do we need this in here? This should surely be handled in the compiler?
> This allows to use those basic annotations of synchronization in Fortran code.
> The annotation interface is also intended for manual instrumenting application code.
> 
> 
> Like for SPEC OMP: 371.applu331/src/syncs.F90 
> 
> ```
> @@ -82,6 +85,8 @@
>  !$omp flush(isync)
>           end do
> +       CALL AnnotateHappensAfter(__FILE__, __LINE__, isync(omp_get_thread_num()))
> +       CALL AnnotateHappensBefore(__FILE__, __LINE__, isync(iam))
>           isync(iam) = 1
>  !$omp flush(isync)
>        endif
> 
> ```
It's not required for Archer and belongs into the sanitizer runtime.


================
Comment at: tools/archer/ompt-tsan.cpp:203
+
+typedef int (*ompt_get_task_memory_t)(void **addr, size_t *size, int blocknum);
+
----------------
This should be standard now.


================
Comment at: tools/archer/ompt-tsan.cpp:544
+
+	if (hasReductionCallback < ompt_set_always) {
+          // We ignore writes inside the barrier. These would either occur during
----------------
Indention looks odd...


================
Comment at: tools/archer/ompt-tsan.cpp:720-721
+
+  // we will use new stack, assume growing down
+  // TsanNewMemory((char*)&FromTask-1024, 1024);
+  if (ToTask->execution == 0) {
----------------
?


================
Comment at: tools/archer/ompt-tsan.cpp:747
+  // Task may be resumed at a later point in time.
+  // TsanHappensBeforeUC(FromTask->GetTaskPtr());
+  TsanHappensBefore(FromTask->GetTaskPtr());
----------------
?


================
Comment at: tools/archer/tests/CMakeLists.txt:5
+
+# Some tests use math functions
+check_library_exists(m sqrt "" LIBARCHER_HAVE_LIBM)
----------------
Do they?


================
Comment at: tools/archer/tests/CMakeLists.txt:33-34
+pythonize_bool(LIBARCHER_HAVE_ARCHER_RUNTIME)
+pythonize_bool(LIBOMP_OMPT_SUPPORT)
+pythonize_bool(LIBOMP_OMPT_OPTIONAL)
+pythonize_bool(LIBOMP_HAVE_LIBM)
----------------
At least these two don't exist here.


================
Comment at: tools/archer/tests/lit.cfg:105-108
+if config.has_ompt:
+    config.available_features.add("ompt")
+    # for callback.h
+    config.test_flags += " -I " + config.test_source_root + "/ompt"
----------------
Is this needed?


================
Comment at: tools/archer/tests/lit.cfg:117-130
+# OMPT Tests
+config.substitutions.append(("%libomp-compile-and-run", \
+    "%libomp-compile && %libomp-run"))
+config.substitutions.append(("%libomp-cxx-compile-and-run", \
+    "%libomp-cxx-compile && %libomp-run"))
+config.substitutions.append(("%libomp-cxx-compile", \
+    "%clangXX %openmp_flags %flags -std=c++11 %s -o %t" + libs))
----------------
Is this needed?


================
Comment at: tools/archer/tests/lit.site.cfg.in:13-15
+config.hwloc_library_dir = "@LIBOMP_HWLOC_LIBRARY_DIR@"
+config.using_hwloc = @LIBOMP_USE_HWLOC@
+config.has_ompt = @LIBOMP_OMPT_SUPPORT@ and @LIBOMP_OMPT_OPTIONAL@
----------------
This should not be needed


================
Comment at: tools/archer/tests/races/critical-unrelated.c:37
+// CHECK:   Write of size 4
+// CHECK: #0 .omp_outlined.
+// CHECK:   Previous write of size 4
----------------
This depends on the compiler used, right? Can the tests be executed by GCC?


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

https://reviews.llvm.org/D45890