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

Thu Dec 9 00:03:14 PST 2021

jmellorcrummey added inline comments.

================
Comment at: openmp/libomptarget/include/ompt-connector.h:53
+//
+// NOTE: This class is intended for use in attribute constructors. therefore,
+// it should be declared within the constructor function to ensure that
----------------
JonChesterfield wrote:
> I don't understand this note
This class is used to communicate between an OMPT implementation in libomptarget and libomp. It is also used to communicate between an OMPT implementation in a device-specific plugin. The decision whether OMPT is enabled or not needs to be made when the library is loaded before any functions in the library are invoked. For that reason, this class is intended to be declared in the constructor for libomptarget or a plugin so that the decision about whether OMPT is supposed to be enabled is known before any interface function in the library is invoked.

================
Comment at: openmp/libomptarget/include/ompt-connector.h:77
+      DP("OMPT: library_ompt_connect = %s\n", library_connect_routine.c_str());
+      void *vptr = dlsym(NULL, library_connect_routine.c_str());
+      library_ompt_connect = reinterpret_cast<library_ompt_connect_t>(
----------------
JonChesterfield wrote:
> error handling missing here
 If the dlsym doesn't return a non-zero value, then the OMPT interface is not enabled. The check for a zero value is in the connect method. If library_ompt_connect is NULL because dlsym returned NULL, then the code in the client library (the one calling connect) won't try to call this function to connect with its server library.

================
Comment at: openmp/libomptarget/plugins/amdgpu/src/ompt_callback.cpp:118
+
+__attribute__((constructor)) static void ompt_init(void) {
+  DP("OMPT: Entering ompt_init\n");
----------------
JonChesterfield wrote:
> initialization order hazard here. When is this supposed to be constructed relative to the other globals?
The only requirement here is that this initialization must occur in a constructor so that libomp can use the pointer set up in lines 123 to initialize OMPT support when the call on line 130 is made so that OMPT support is either initialized or not before any entry point to libomptarget is invoked.

================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:91
+
+static thread_local uint64_t ompt_target_region_id = 1;
+static thread_local uint64_t ompt_target_region_opid = 1;
----------------
JonChesterfield wrote:
> Why are these all global variables?
These are global (but thread local) variables because they hold persistent state used by a thread operating in a target region.
When you enter a target region these variables are initialized. The operation id is adjusted every call to a target region operation. ompt_task_data and ompt_target_task_data are set when the region is entered and persist throughout a target region being performed by a thread.

Note that these variables are outside the ompt_interface class because they are manipulated by functions passed to methods such as  ompt_device_callbacks.ompt_callback_target_data_op_emi. 

================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:242
+
+void OmptInterface::target_data_retrieve_begin(int64_t device_id,
+                                               void *hst_ptr_begin,
----------------
JonChesterfield wrote:
> Can we factor this so copy&paste typos are less likely?
The commonality is between begin/end pairs.  What would you propose? Factoring out an EMI call into a function F that can be called with a few less arguments so that global variables like ompt_target_task_data, ompt_target_data don't need to be passed to be passed to F? I don't think this adds much simplification.

================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:407
+
+__attribute__((constructor(102))) static void ompt_init(void) {
+  static library_ompt_connector_t libomp_connector("libomp");
----------------
JonChesterfield wrote:
> This sort of thing makes the library awkward to construct safely. I guess 102 is related to the other function tagged constructor, but we're better off exposing a single library_init function which is called by libomptarget at the appropriate point in time
While this could be a global function called at the end of the other constructor in rtl.cpp, this would be the only reason any OMPT header has to be included in rtl.cpp.

The other option is to declare all of the priorities passed to constructors in one place and make the number for this constructor the successor to the other constructor, e.g.

#define RTL_CONSTRUCTOR_PRIORITY 101
#define OMPT_CONSTRUCTOR_PRIORITY (RTL_CONSTRUCTOR_PRIORITY + 1)

Using these defines means that no OMPT header file needs to be included in rtl.cpp. Or perhaps you'd prefer a constructor in its own file that calls the "constructor" functions in rtl.cpp and ompt_callback.cpp and the constructor can include headers for rtl.cpp and ompt_callback.cpp that define the functions that need to be called by the single constructor.

================
Comment at: openmp/runtime/src/ompt-specific.cpp:269
+  lwt->ompt_task_info.frame.enter_frame_flags = 0;
+  ;
   lwt->ompt_task_info.frame.exit_frame = ompt_data_none;
----------------
JonChesterfield wrote:
> ?
I'm not sure what the question is. 

The enter_frame_flags and exit_frame_flags used by the runtime entry points for inspecting frame objects provide information about how to locate the frame pointer of a procedure frame that called into or out of the runtime. 

When setting up a lightweight task, these flags are cleared. 

They will be set later to a logical or 

either ompt_frame_runtime or ompt_frame_application

ored together with either ompt_frame_framepointer or ompt_frame_cfa

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728