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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Skimmed through this patch. Lots of code that looks like it was copy + paste + modify which is usually a bug farm. Quite a lot of global variables. No associated tests.

Lack of tests seems the most significant problem here.



================
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
----------------
I don't understand this note


================
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>(
----------------
error handling missing here


================
Comment at: openmp/libomptarget/include/omptarget.h:351
 
-void __kmpc_push_target_tripcount(int64_t device_id, uint64_t loop_tripcount);
+void __kmpc_push_target_tripcount(ident_t *loc, int64_t device_id,
+                                  uint64_t loop_tripcount);
----------------
I thought these functions were ABI stable, i.e. we can't change them


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/ompt_callback.cpp:118
+
+__attribute__((constructor)) static void ompt_init(void) {
+  DP("OMPT: Entering ompt_init\n");
----------------
initialization order hazard here. When is this supposed to be constructed relative to the other globals?


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:42
+
+class libomptarget_rtl_finalizer_t : std::list<ompt_finalize_t> {
+public:
----------------
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?


================
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;
----------------
Why are these all global variables?


================
Comment at: openmp/libomptarget/src/ompt_callback.cpp:242
+
+void OmptInterface::target_data_retrieve_begin(int64_t device_id,
+                                               void *hst_ptr_begin,
----------------
Can we factor this so copy&paste typos are less likely?


================
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");
----------------
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


================
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;
----------------
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728