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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

Read the large comment first, this might also result in changes to other patches. The design should match nextgen and it should be a proper part of it, not some attached afterthought.



================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:149
+  /// Documentation based on omp-tools
+  static const char *Documentation;
 };
----------------
I don'n understand why there are so many static members and functions everywhere, but alas.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:30
+/// Array denoting the devices
+static OmptDeviceTy *Devices = nullptr;
+
----------------
Now there are static members, and static non members, I mean, why?


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:29
+/// Array denoting the devices
+static OmptDeviceTy *Devices = 0;
+
----------------
mhalk wrote:
> jplehr wrote:
> > Probably better use `nullptr` here.
> Done.
Also above?


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:366
+#ifdef OMPT_SUPPORT
+  OmptDeviceCallbacks.prepareDevices(Plugin.getNumDevices());
+  OmptDeviceCallbacks.OmptCallbackDeviceInitialize(
----------------
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.




================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:470-473
+      DeviceId, nullptr /* FileName */, 0 /* File Offset */,
+      nullptr /* VmaInFile */, Bytes /* ImgSize */,
+      InputTgtImage->ImageStart /* HostAddr */, nullptr /* DeviceAddr */,
+      0 /* FIXME: ModuleId */);
----------------
mhalk wrote:
> jplehr wrote:
> > Idk if there is a guideline for it, but I personally would prefer to have this read as `/* FileName= */ nullptr, ...`
> I would be interested in that, too -- in the previous version of these files it was this (`nullptr /* FileName= */,`) way.
> But TBH I don't find that intuitively either, i.e. I would also prefer your proposal.
> Any other thoughts or info on this would be appreciated.
`/* Name */ Value`, which is hopefully what the rest of the code does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652