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

Joachim Protze via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Fri Nov 12 07:24:15 PST 2021

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
+# Duplicated from openmp/runtime/cmake/config-ix.cmake
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:


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

Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:63
+  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_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
+// 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`:

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.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list