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

Dhruva Chakrabarti via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Thu Dec 9 19:26:51 PST 2021

dhruvachak marked 4 inline comments as done.
dhruvachak added inline comments.

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>(
jmellorcrummey wrote:
> 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.
I deleted the default constructor in the next version so as to prevent its accidental use in new code. This will ensure proper initialization, no other error handling required here.

Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:43
+#include <ompt_device_callbacks.h>
+#define OMPT_IF_ENABLED(stmts)                                                 \
+  if (ompt_device_callbacks.is_enabled()) {                                    \
JonChesterfield wrote:
> This is an unguarded if statement, will interact badly with other if statements next to it in code. Macros like this are traditionally wrapped in `do { } while (0)` to avoid that syntax hazard
Fixed in the next version.

Comment at: openmp/libomptarget/src/ompt_callback.cpp:42
+class libomptarget_rtl_finalizer_t : std::list<ompt_finalize_t> {
JonChesterfield wrote:
> I can't remember if private inheritance of std containers is well defined or not. This class doesn't seem to use any of the members of std::list. What's going on here?
Removed in the next version.

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list