[Openmp-commits] [PATCH] D124070: [OpenMP] [OMPT] [amdgpu] [4/8] Implemented callback registration in nextgen plugins

[Dhruva Chakrabarti via Phabricator via Openmp-commits]

dhruvachak added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:67
+  /// Connect plugin instance with libomptarget
+  static OmptLibraryConnectorTy LibomptargetConnector("libomptarget");
+  static ompt_start_tool_result_t OmptResult;
----------------
I think LibomptargetConnector and OmptResult need not be statics, locals will suffice. The connector object is used to copy the callback function pointers from libomptarget and once that is done, the object is not required any more. The original intent of the static was to guard against accidental repeat connect attempts. If we add the assert to the finalizer like I suggested, that will still be guarded against. Regarding OmptResult, it is used to communicate some pointers to the other library which caches the pointers, so no need to keep the wrapper object around.

We could make similar changes to the connect logic in libomptarget and add an assert during finalizer registration in libomp but let's keep that for a separate commit, if at all.


================
Comment at: openmp/libomptarget/src/OmptCallback.cpp:39
+  LibomptargetRtlFinalizer() : FiniFn(nullptr) {}
+  void registerRtl(ompt_finalize_t fn) { FiniFn = fn; }
+  void finalize() {
----------------
In registerRtl, before assigning to FiniFn, add an assert that it is nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124070