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

[Dhruva Chakrabarti via Phabricator via Openmp-commits]

dhruvachak marked 8 inline comments as done.
dhruvachak added a comment.

Thanks, Joachim, for your comments. I removed the unrelated changes and made the cmake change in the next version.



================
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)
----------------
protze.joachim wrote:
> 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
> 
Added the parent scope.


================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:63
+public:
+  virtual void ompt_callback_device_initialize(int device_num,
+                                               const char *type) {
----------------
protze.joachim wrote:
> Why are the functions virtual?
I think the intent was to have plugin specific derived classes but that's not the case at the moment. So removed virtual. 


================
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
 
----------------
protze.joachim wrote:
> These changes seem unrelated.
 OMPT_FRAME_SET is called by __ompt_set_frame_enter_internal which is part of target region support. __ompt_set_frame_enter_internal is called by ompt_set_frame_enter which is used by ompt_callback.


================
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
----------------
protze.joachim wrote:
> 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?
Since it's unrelated, I removed it.


================
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;
----------------
protze.joachim wrote:
> @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`
Made the suggested change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728