[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.

When more changes have accumulated I'll update this diff with the changes I replied to with 'Done'.



================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:52
+  /// Invoked when a device is initialized
+  void OmptCallbackDeviceInitialize(int DeviceNum, const char *Type) {
+    if (ompt_callback_device_initialize_fn) {
----------------
jplehr wrote:
> If I recall correctly, methods should be formulated more in active voice, e.g., `initializeOmptCallbackDevice`, and start with a lower-case letter. I believe this is true for the other methods in this class as well.
Agreed.
If more people request this NFC, I'll gladly incorporate this in the patch stack.
I decided against this, since I wanted to concentrate on the core of the patches and not defer them any longer.


================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:141
+  /// Allocate devices
+  static void resize(int NumDevices);
+  /// Deallocate devices
----------------
jplehr wrote:
> I think this should be renamed to `allocate` or so, as the comment suggests that it allocates and does not resize.
Agreed. Done.


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


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:32
+void OmptDeviceCallbacksTy::resize(int NumDevices) {
+  Devices = new OmptDeviceTy[NumDevices];
+}
----------------
jplehr wrote:
> Potentially we should not allocate if `Devices` already holds a pointer, i.e., is not `nullptr`? This looks suspiciously as if we can leak memory here.
Agreed. I'll simply send a DebugPrint when allocation failed.
That is, when: zero or negative NumDevices were requested or when `Devices` was already allocated / non-nullptr.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:38
+OmptDeviceTy *OmptDeviceCallbacksTy::lookupDevice(int DeviceNum) {
+  return &Devices[DeviceNum];
+}
----------------
jplehr wrote:
> Do we want / need to guard against out-of-bounds access here?
I think we should.
Done.


================
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 */);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652