[Openmp-commits] [PATCH] D113728: [libomptarget] [amdgpu] Foundation for OMPT target callback support

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a subscriber: AndreyChurbanov.
protze.joachim added a comment.

I mainly checked the libomp code and put some inline comments.



================
Comment at: openmp/libomptarget/CMakeLists.txt:66-89
+# Let libomptarget OMPT support enabling follow host OMPT support
+# But LIBOMP_ARCH is not set
+libomp_get_architecture(LIBOMP_ARCH)
+
+# Duplicated from openmp/runtime/cmake/config-ix.cmake
+if(NOT LIBOMP_HAVE___BUILTIN_FRAME_ADDRESS)
+  set(LIBOMP_HAVE_OMPT_SUPPORT FALSE)
----------------
This code does not reproduce the libomp control flow for in-llvm-tree and standalone build.

To use cmake variables from openmp/runtime, just make them visible in parent scope:

set(LIBOMP_HAVE_OMPT_SUPPORT ${LIBOMP_HAVE_OMPT_SUPPORT} PARENT_SCOPE)

see https://github.com/llvm/llvm-project/commit/3bc8ce5dd718beef0031bf4b070ac4026e6910d7#diff-01127988bc2b1599723b049e94ada3025063f4e0a7bac326aaf37b892a118fd1R332



================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:63
+public:
+  virtual void ompt_callback_device_initialize(int device_num,
+                                               const char *type) {
----------------
Why are the functions virtual?


================
Comment at: openmp/runtime/src/ompt-general.cpp:504-509
+      OMPT_FRAME_SET(task_frame, exit, OMPT_GET_FRAME_ADDRESS(0),
+                     (ompt_frame_runtime | OMPT_FRAME_POSITION_DEFAULT));
       ompt_callbacks.ompt_callback(ompt_callback_implicit_task)(
-          ompt_scope_begin, parallel_data, task_data, 1, 1, ompt_task_initial);
+          ompt_scope_begin, parallel_data /*parallel data*/, task_data,
+          1 /*team size*/, 1 /*initial task: index=1*/, ompt_task_initial);
+      OMPT_FRAME_CLEAR(task_frame, exit);
----------------
This change seems unrelated and it is not obvious whether this change is correct.


================
Comment at: openmp/runtime/src/ompt-internal.h:16-37
+#include "kmp_platform.h"
+
 #include "ompt-event-specific.h"
 #include "omp-tools.h"
 
 #define OMPT_VERSION 1
 
----------------
These changes seem unrelated.


================
Comment at: openmp/runtime/src/ompt-internal.h:119-135
+
+#if (KMP_ARCH_PPC64 | KMP_ARCH_ARM)
+// On Power and ARM, the frame pointer (__builtin_frame_address(0))
+// points to the top of the stack frame. For gcc4, this is not a useful
+// value after returning from GOMP_parallel_start to call an outlined
+// task in the master thread. To support gcc4 in a uniform fashion,
+// always use the canonical frame address (known as CFA, which is the
----------------
These are unrelated changes.


================
Comment at: openmp/runtime/src/ompt-internal.h:128
+// __builtin_frame_address(1), for the OMPT frame pointer for a frame.
+#define OMPT_GET_FRAME_ADDRESS(level) __builtin_frame_address(level + 1)
+#define OMPT_FRAME_POSITION_DEFAULT ompt_frame_cfa
----------------
>From __builtin_frame_address reference:
> Calling this function with a nonzero argument can have unpredictable effects, including crashing the calling program. As a result, calls that are considered unsafe are diagnosed when the -Wframe-address option is in effect. Such calls should only be made in debugging situations.

How should we deal with this restriction?


================
Comment at: openmp/runtime/src/ompt-specific.cpp:361
+  // an OMPT tool
+  static thread_local ompt_data_t target_task_data;
+  return &target_task_data;
----------------
@AndreyChurbanov what do you think about the use of `thread_local` in libomp? 

If a thread-local value is sufficient (which I doubt from OMPT perspective), this value could be stored in a new entry of `ompt_thread_info_t`:
`__kmp_threads[__kmp_get_gtid()]->th.ompt_thread_info.target_data`


================
Comment at: openmp/runtime/src/ompt-specific.h:87-107
+#define OMPT_STORE_RETURN_ADDRESS_GCC4(gtid)                                   \
   if (ompt_enabled.enabled && gtid >= 0 && __kmp_threads[gtid] &&              \
       !__kmp_threads[gtid]->th.ompt_thread_info.return_address)                \
   __kmp_threads[gtid]->th.ompt_thread_info.return_address =                    \
-      __builtin_return_address(0)*/
+      __builtin_return_address(0)
+
 #define OMPT_STORE_RETURN_ADDRESS(gtid)                                        \
----------------
These are unrelated changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728