[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:470-473
+      DeviceId, nullptr /* FileName */, 0 /* File Offset */,
+      nullptr /* VmaInFile */, Bytes /* ImgSize */,
+      InputTgtImage->ImageStart /* HostAddr */, nullptr /* DeviceAddr */,
+      0 /* FIXME: ModuleId */);
----------------
jdoerfert wrote:
> 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.
Yes, that is the guideline. /* Name=*/Value
See https://llvm.org/docs/CodingStandards.html#commenting


================
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,
----------------
Is there a reason you need to pass in the GenericPluginTy here? I don't see it getting used.


================
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)                                                     \
----------------
This looks unrelated. Can you verify whether we really need this for this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124652