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

[Jan-Patrick Lehr via Phabricator via Openmp-commits]

jplehr added a comment.

Thanks for moving this forward.



================
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) {
----------------
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.


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


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


================
Comment at: openmp/libomptarget/plugins-nextgen/common/OMPT/OmptCallback.cpp:32
+void OmptDeviceCallbacksTy::resize(int NumDevices) {
+  Devices = new OmptDeviceTy[NumDevices];
+}
----------------
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.


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


================
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 */);
----------------
Idk if there is a guideline for it, but I personally would prefer to have this read as `/* FileName= */ nullptr, ...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652