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

[Dhruva Chakrabarti via Phabricator via Openmp-commits]

dhruvachak added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:366
+#ifdef OMPT_SUPPORT
+  OmptDeviceCallbacks.prepareDevices(Plugin.getNumDevices());
+  OmptDeviceCallbacks.OmptCallbackDeviceInitialize(
----------------
jdoerfert wrote:
> So, these OmptDevices are stored in 4 or so static members mostly inside OmptDeviceCallbacksTy, and initialized via a member of OmptDeviceCallbacks which is a global object that flies around.
> 
> This is really too much.
> 
> Let's approach this differently:
> One OmptDeviceTy per device of a plugin, right?
> If so, make one such object a member of GenericDeviceTy, all that array stuff and 4 functions can be removed, init and deinit are performed via constructor/destructor or init/deinit of the parent.
> Funnily, once you do that, the entire `OmptDeviceTy` code would be moot and all you really put into GenericDeviceTy::(de)init is one conditional to check and call the callback.
> You can even make a variadic macro that checks the callback and calls it, forwarding the args, which will simplify the pattern to sth like:
> ```
> ompt_callback(device_initialize, DeviceNum, Type, reinterpret_cast<ompt_device_t *>(this), doLookup, Documentation);
> ```
> 
> Then, why do we need OmptDeviceCallbacksTy in the first place?
> Shouldn't the callbacks be close to their invocation, e.g., in GenericDeviceTy, etc.
> 
> 
I agree all or most of those statics can go away. There is a single instance of OmptDeviceCallbacksTy in libomptarget and another instance in the plugin. So those statics are redundant.

About OmptDeviceCallbacksTy, I suggest splitting it up into 2 structs. The first, OmptHostEventsCallbacksTy would contain the host callback function ptrs. The second, OmptDeviceEventsCallbacksTy would contain the 4 device callback function ptrs: load, unload, init, and fini. This is along the lines of the categories already present in omp-tools.h. Then, libomptarget would instantiate only OmptHostEventsCallbacksTy by using the "connect" interface to libomp. Similarly, the plugin would instantiate only OmptDeviceEventsCallbacksTy by using the "connect" interface to libomp. (In the current patch, the plugin connects to libomptarget, that will have to be changed.) The object of type OmptDeviceEventsCallbacksTy can reside in GenericDeviceTy. (Later on, when we introduce support for trace records, we will need new OMPT entry points that the tool can call. Examples: ompt_start_trace, ompt_flush_trace, etc. At that point, another struct OmptDeviceEntryPointsTy holding those entry points can be introduced, residing in GenericDeviceTy. But that's for later.) The object of type OmptHostEventsCallbacksTy can stay as a global object like in current trunk unless we find a better place for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652