[Openmp-commits] [PATCH] D113728: [libomptarget] [amdgpu] Foundation for OMPT target callback support

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/include/ompt_device_callbacks.h:2
+//=== ompt_device_callbacks.h - Target independent OpenMP target RTL -- C++
+//-===//
+//
----------------
dhruvachak wrote:
> jdoerfert wrote:
> > Nit: Style
> clang-format makes this change and I left it alone.
Your line is too long. "Target independent OpenMP target RTL" can be shortened, maybe even replaced with something that explains what this file does. I have no idea what "Target independent OpenMP target RTL" means wrt. OMPT callbacks. My guess is this was copied from somewhere else.


================
Comment at: openmp/libomptarget/src/device.cpp:500
+  OMPT_IF_BUILT(OmptInterfaceTargetDataOpRAII TgtDataAlloc(
+      RTLDeviceID, Size, HstPtr, nullptr /* TgtPtr */, ompt_target_data_alloc));
   return RTL->data_alloc(RTLDeviceID, Size, HstPtr, Kind);
----------------
For these RAIIs, you could put OMPT_IF_BUILT into the RAII code and make them a no-op if OMPT is not used. That way you don't need any macro here, the RAII would always be there and removed when build w/o OMPT.


================
Comment at: openmp/libomptarget/src/device.cpp:536
+  }
 }
 
----------------
No need for the extra indention.


================
Comment at: openmp/libomptarget/src/device.cpp:562
+  }
 }
 
----------------
same as above.


================
Comment at: openmp/libomptarget/src/interface.cpp:130
+  }
 }
 
----------------
same as above, I'll stop pointing it out now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113728