[Openmp-commits] [PATCH] D124652: [OpenMP] [OMPT] [amdgpu] [5/8] Implemented device init/fini/load callbacks

[Michael Halkenhäuser via Phabricator via Openmp-commits]

mhalk added a comment.

Thanks @dhruvachak I'll rebase (esp. w.r.t. the deletion of the legacy plugins), then check the upcoming changes and if I don't see problems: submit here.



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:476
 
-Error GenericDeviceTy::deinit() {
-  // Delete the memory manager before deinitilizing the device. Otherwise,
+Error GenericDeviceTy::deinit(GenericPluginTy &Plugin) {
+  // Delete the memory manager before deinitializing the device. Otherwise,
----------------
dhruvachak wrote:
> Is there a reason you need to pass in the GenericPluginTy here? I don't see it getting used.
Very good catch, this *was* needed initally, when the init and fini was done within the generic plugin.
But this change is not necessary anymore as the patch has undergone a lot of changes since then.


================
Comment at: openmp/runtime/src/ompt-general.cpp:883
 static ompt_interface_fn_t ompt_libomp_target_fn_lookup(const char *s) {
+#define provide_fn(fn)                                                         \
+  if (strcmp(s, #fn) == 0)                                                     \
----------------
dhruvachak wrote:
> This looks unrelated. Can you verify whether we really need this for this patch?
Yes, we actually use this in this patch, to provide access to a "callback function lookup by code" (rather than name):
`ompt_get_callback_t lookupCallbackByCode`
It will be used to bind the actual OMPT callback functions to their declarations.

My idea was: This should provide a lookup with less overhead, than by name (comparing integers vs. `strcmp`, esp. since the function names are prefixed with `ompt_callback_`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652