[Openmp-commits] [PATCH] D99803: [openmp] Add OMPT initialization in libomptarget

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added inline comments.


================
Comment at: openmp/libomptarget/src/rtl.cpp:69
+    DP("Init OMPT for libomptarget\n");
+    libomptarget_start_tool_t start_tool = (libomptarget_start_tool_t)dlsym(
+        RTLD_DEFAULT, "libomptarget_start_tool");
----------------
Below in line 103, `__kmpc_get_target_offload` is called directly without dlsym. Is that the preferred way to call into libomp?


================
Comment at: openmp/libomptarget/src/rtl.cpp:80
+    }
+    ompt_initialized = true;
+  }
----------------
grokos wrote:
> If the call to `start_tool` failed, is it correct to claim that ompt is initialized? On the other hand, maybe it's enough that you've set `ompt_target_enabled` to 0.
The OMPT initialization mechanism should happen once during the execution of an application. 
Is there a scenario, where this code might execute twice, so that the if(!ompt_initialized) might have any effect?

I just realized, that we will need to check what libomp is doing after forking a process, but that's a separate issue.


================
Comment at: openmp/runtime/src/ompt-general.cpp:133
 
+_OMP_EXTERN OMPT_WEAK_ATTRIBUTE bool libomptarget_start_tool(
+    ompt_target_callbacks_active_t *libomptarget_ompt_enabled,
----------------
This function is called by libomptarget and should not be weak.


================
Comment at: openmp/runtime/src/ompt-general.cpp:137-139
+  if (!TCR_4(__kmp_init_middle)) {
+    __kmp_middle_initialize();
+  }
----------------
This will call `__kmp_middle_initialize` whenever libomptarget gets initialized. Is this ok?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99803