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

[Dhruva Chakrabarti via Phabricator via Openmp-commits]

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> {
+public:
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728