[OMPT] Avoid unused allocations & code clean up (#378)

This moves the allocation for ProfilerSpecificData into the
TraceGeneratorRAII object, thus, it only allocates memory for events
that are actually traced.

This is the result of @dhruvachak comment on my previous OMPT PR. While
we weren't leaking memory, we would do extra allocations which would
accumulate and not be freed. This patch addresses that issue. The
allocation is now done inside `TraceInterfaceRAII` in Interface.h when
the event has _not_ been filtered.
Tested on single and multi GPU systems.

The way that the higher-level (Interface.h / device.cpp` talks to the
lower-level (plugins) is unchanged through this PR and I'm still looking
at it / thinking about it.

It also does a bit of NFCI code clean up that can be split out of this
commit if desired.

Author: jplehr

offload/include/OpenMP/OMPT/Interface.h

@@ -21,6 +21,8 @@
 #include "Shared/Debug.h"
 #include "omp-tools.h"
 
+#include "GenericProfiler.h"
+
 #include "llvm/Support/ErrorHandling.h"
 
 #include <functional>
@@ -473,16 +475,20 @@ template <typename FunctionPairTy, typename AsyncInfoTy, typename... ArgsTy>
 class TracerInterfaceRAII {
 public:
   TracerInterfaceRAII(FunctionPairTy Callbacks, AsyncInfoTy &AsyncInfo,
-                      void *OmptSpecificData, int TracedDeviceId,
+                      plugin::GenericProfilerTy *Prof, int TracedDeviceId,
                       ompt_callbacks_t EventType, ArgsTy... Args)
       : Arguments(Args...), beginFunction(std::get<0>(Callbacks)) {
     __tgt_async_info *AI = AsyncInfo;
     if (isTracingEnabled(TracedDeviceId, EventType)) {
       auto Record = begin();
 
-      // Access the already allocated profiler data and populate it
+      // The Profiler can allocate specific data to be used to pass information
+      // from here to lower parts of the runtime system.
+      // NOTE: It is the responsibility of the programmer to ensure type
+      // compatibility and correct usage of the data. The profiler, however,
+      // OWNS the pointer and frees it at an appropriate time.
       OmptEventInfoTy *ProfilerData =
-          reinterpret_cast<OmptEventInfoTy *>(OmptSpecificData);
+          reinterpret_cast<OmptEventInfoTy *>(Prof->getProfilerSpecificData());
       ProfilerData->TraceRecord = Record;
       ProfilerData->NumTeams = 0;
 

offload/libomptarget/device.cpp

@@ -245,18 +245,12 @@ int32_t DeviceTy::submitData(void *TgtPtrBegin, void *HstPtrBegin, int64_t Size,
           RegionInterface.getCallbacks<ompt_target_data_transfer_to_device>(),
           omp_get_initial_device(), HstPtrBegin, DeviceID, TgtPtrBegin, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);
-      // The Profiler can allocate specific data to be used to pass information
-      // from here to lower parts of the runtime system.
-      // NOTE: It is the responsibility of the programmer to ensure type
-      // compatibility and correct usage of the data. The profiler, however,
-      // OWNS the pointer and frees it at an appropriate time.
-      void *ProfData = RTL->getProfiler()->getProfilerSpecificData();
       // Only if 'TracedDeviceId' is actually traced, AsyncInfo->OmptEventInfo
       // is set and a trace record generated. Otherwise: No OMPT device tracing.
       TracerInterfaceRAII TargetDataSubmitTraceRAII(
           RegionInterface
               .getTraceGenerators<ompt_target_data_transfer_to_device>(),
-          AsyncInfo, ProfData, /*TracedDeviceId=*/DeviceID,
+          AsyncInfo, RTL->getProfiler(), /*TracedDeviceId=*/DeviceID,
           /*EventType=*/ompt_callback_target_data_op, omp_get_initial_device(),
           HstPtrBegin, DeviceID, TgtPtrBegin, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);)
@@ -281,18 +275,12 @@ int32_t DeviceTy::retrieveData(void *HstPtrBegin, void *TgtPtrBegin,
           RegionInterface.getCallbacks<ompt_target_data_transfer_from_device>(),
           DeviceID, TgtPtrBegin, omp_get_initial_device(), HstPtrBegin, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);
-      // The Profiler can allocate specific data to be used to pass information
-      // from here to lower parts of the runtime system.
-      // NOTE: It is the responsibility of the programmer to ensure type
-      // compatibility and correct usage of the data. The profiler, however,
-      // OWNS the pointer and frees it at an appropriate time.
-      void *ProfData = RTL->getProfiler()->getProfilerSpecificData();
       // Only if 'TracedDeviceId' is actually traced, AsyncInfo->OmptEventInfo
       // is set and a trace record generated. Otherwise: No OMPT device tracing.
       TracerInterfaceRAII TargetDataSubmitTraceRAII(
           RegionInterface
               .getTraceGenerators<ompt_target_data_transfer_from_device>(),
-          AsyncInfo, ProfData, /*TracedDeviceId=*/DeviceID,
+          AsyncInfo, RTL->getProfiler(), /*TracedDeviceId=*/DeviceID,
           /*EventType=*/ompt_callback_target_data_op, DeviceID, TgtPtrBegin,
           omp_get_initial_device(), HstPtrBegin, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);)
@@ -316,18 +304,12 @@ int32_t DeviceTy::dataExchange(void *SrcPtr, DeviceTy &DstDev, void *DstPtr,
           RegionInterface.getCallbacks<ompt_target_data_transfer_from_device>(),
           RTLDeviceID, SrcPtr, DstDev.RTLDeviceID, DstPtr, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);
-      // The Profiler can allocate specific data to be used to pass information
-      // from here to lower parts of the runtime system.
-      // NOTE: It is the responsibility of the programmer to ensure type
-      // compatibility and correct usage of the data. The profiler, however,
-      // OWNS the pointer and frees it at an appropriate time.
-      void *ProfData = RTL->getProfiler()->getProfilerSpecificData();
       // Only if 'TracedDeviceId' is actually traced, AsyncInfo->OmptEventInfo
       // is set and a trace record generated. Otherwise: No OMPT device tracing.
       TracerInterfaceRAII TargetDataExchangeTraceRAII(
           RegionInterface
               .getTraceGenerators<ompt_target_data_transfer_from_device>(),
-          AsyncInfo, ProfData, /*TracedDeviceId=*/RTLDeviceID,
+          AsyncInfo, RTL->getProfiler(), /*TracedDeviceId=*/RTLDeviceID,
           /*EventType=*/ompt_callback_target_data_op, RTLDeviceID, SrcPtr,
           DstDev.RTLDeviceID, DstPtr, Size,
           /*CodePtr=*/OMPT_GET_RETURN_ADDRESS);)

offload/libomptarget/omptarget.cpp

@@ -2101,13 +2101,6 @@ int target(ident_t *Loc, DeviceTy &Device, void *HostPtr,
     InterfaceRAII TargetSubmitRAII(
         RegionInterface.getCallbacks<ompt_callback_target_submit>(), NumTeams);
 
-    // The Profiler can allocate specific data to be used to pass information
-    // from here to lower parts of the runtime system.
-    // NOTE: It is the responsibility of the programmer to ensure type
-    // compatibility and correct usage of the data. The profiler, however, OWNS
-    // the pointer and frees it at an appropriate time.
-    void *ProfData = Device.RTL->getProfiler()->getProfilerSpecificData();
-
     // Calls "begin" for the OMPT trace record and let the plugin
     // enqueue the stop operation for after the kernel is done. The stop
     // operation completes the trace record entry with the information from
@@ -2116,7 +2109,7 @@ int target(ident_t *Loc, DeviceTy &Device, void *HostPtr,
     // set and a trace record generated. Otherwise: No OMPT device tracing.
     TracerInterfaceRAII TargetTraceRAII(
         RegionInterface.getTraceGenerators<ompt_callback_target_submit>(),
-        AsyncInfo, ProfData, /*TracedDeviceId=*/DeviceId,
+        AsyncInfo, Device.RTL->getProfiler(), /*TracedDeviceId=*/DeviceId,
         /*EventType=*/ompt_callback_target_submit, DeviceId, NumTeams);
 #endif
     Ret = Device.launchKernel(TgtEntryPtr, TgtArgs.data(), TgtOffsets.data(),

offload/plugins-nextgen/common/OMPT/OmptProfiler.cpp

@@ -11,12 +11,14 @@
 //===----------------------------------------------------------------------===//
 
 #include "OmptProfiler.h"
+#include "OpenMP/OMPT/Interface.h"
 #include "PluginInterface.h"
 #include "Shared/Debug.h"
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleInit(
-    llvm::omp::target::plugin::GenericDeviceTy *Device,
-    llvm::omp::target::plugin::GenericPluginTy *Plugin) {
+using namespace llvm::omp::target;
+
+void ompt::OmptProfilerTy::handleInit(plugin::GenericDeviceTy *Device,
+                                      plugin::GenericPluginTy *Plugin) {
   auto DeviceId = Device->getDeviceId();
   auto DevicePtr = reinterpret_cast<ompt_device_t *>(Device);
   ompt::setDeviceId(DevicePtr, Plugin->getUserId(DeviceId));
@@ -32,9 +34,8 @@ void llvm::omp::target::ompt::OmptProfilerTy::handleInit(
   }
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleDeinit(
-    llvm::omp::target::plugin::GenericDeviceTy *Device,
-    llvm::omp::target::plugin::GenericPluginTy *Plugin) {
+void ompt::OmptProfilerTy::handleDeinit(
+    plugin::GenericDeviceTy *Device, target::plugin::GenericPluginTy *Plugin) {
   auto DeviceId = Device->getDeviceId();
 
   if (ompt::Initialized) {
@@ -45,50 +46,55 @@ void llvm::omp::target::ompt::OmptProfilerTy::handleDeinit(
   ompt::removeDeviceId(reinterpret_cast<ompt_device_t *>(Device));
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleLoadBinary(
-    llvm::omp::target::plugin::GenericDeviceTy *Device,
-    llvm::omp::target::plugin::GenericPluginTy *Plugin,
-    const StringRef InputTgtImage) {
-  auto DeviceId = Device->getDeviceId();
+void ompt::OmptProfilerTy::handleLoadBinary(plugin::GenericDeviceTy *Device,
+                                            plugin::GenericPluginTy *Plugin,
+                                            const StringRef InputTgtImage) {
 
-  if (ompt::Initialized) {
-    size_t Bytes = InputTgtImage.size();
-    performOmptCallback(
-        device_load, Plugin->getUserId(DeviceId),
-        /*FileName=*/nullptr, /*FileOffset=*/0, /*VmaInFile=*/nullptr,
-        /*ImgSize=*/Bytes,
-        /*HostAddr=*/const_cast<unsigned char *>(InputTgtImage.bytes_begin()),
-        /*DeviceAddr=*/nullptr, /* FIXME: ModuleId */ 0);
-  }
+  if (!ompt::Initialized)
+    return;
+
+  auto DeviceId = Device->getDeviceId();
+  size_t Bytes = InputTgtImage.size();
+  performOmptCallback(
+      device_load, Plugin->getUserId(DeviceId),
+      /*FileName=*/nullptr, /*FileOffset=*/0, /*VmaInFile=*/nullptr,
+      /*ImgSize=*/Bytes,
+      /*HostAddr=*/const_cast<unsigned char *>(InputTgtImage.bytes_begin()),
+      /*DeviceAddr=*/nullptr, /* FIXME: ModuleId */ 0);
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleDataAlloc(
-    uint64_t StartNanos, uint64_t EndNanos, void *HostPtr, uint64_t Size,
-    void *Data) {
+void ompt::OmptProfilerTy::handleDataAlloc(uint64_t StartNanos,
+                                           uint64_t EndNanos, void *HostPtr,
+                                           uint64_t Size, void *Data) {
   ompt::setOmptTimestamp(StartNanos, EndNanos);
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleDataDelete(
-    uint64_t StartNanos, uint64_t EndNanos, void *TgtPtr, void *Data) {
+void ompt::OmptProfilerTy::handleDataDelete(uint64_t StartNanos,
+                                            uint64_t EndNanos, void *TgtPtr,
+                                            void *Data) {
   ompt::setOmptTimestamp(StartNanos, EndNanos);
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handlePreKernelLaunch(
-    llvm::omp::target::plugin::GenericDeviceTy *Device, uint32_t NumBlocks[3],
+void ompt::OmptProfilerTy::handlePreKernelLaunch(
+    plugin::GenericDeviceTy *Device, uint32_t NumBlocks[3],
     __tgt_async_info *AI) {
-  OMPT_IF_TRACING_ENABLED(
-      if (llvm::omp::target::ompt::isTracedDevice(getDeviceId(Device))) {
-        if (AI->ProfilerData != nullptr) {
-          auto ProfilerSpecificData = reinterpret_cast<ompt::OmptEventInfoTy *>(AI->ProfilerData);
-          // Set number of granted teams for OMPT
-          setOmptGrantedNumTeams(NumBlocks[0]);
-          ProfilerSpecificData->NumTeams = NumBlocks[0];
-        }
-      });
+  if (!ompt::isTracedDevice(getDeviceId(Device)))
+    return;
+
+  if (AI->ProfilerData == nullptr)
+    return;
+
+  auto ProfilerSpecificData =
+      reinterpret_cast<ompt::OmptEventInfoTy *>(AI->ProfilerData);
+  assert(ProfilerSpecificData && "Invalid ProfilerSpecificData");
+  // Set number of granted teams for OMPT
+  setOmptGrantedNumTeams(NumBlocks[0]);
+  ProfilerSpecificData->NumTeams = NumBlocks[0];
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleKernelCompletion(
-    uint64_t StartNanos, uint64_t EndNanos, void *Data) {
+void ompt::OmptProfilerTy::handleKernelCompletion(uint64_t StartNanos,
+                                                  uint64_t EndNanos,
+                                                  void *Data) {
 
   if (!isProfilingEnabled())
     return;
@@ -98,24 +104,24 @@ void llvm::omp::target::ompt::OmptProfilerTy::handleKernelCompletion(
   if (!Data)
     return;
 
-  DP("OMPT-Async: Time kernel for asynchronous execution (Plugin): Start %lu "
+  DP("OMPT-Async: Time kernel for asynchronous execution: Start %lu "
      "End %lu\n",
      StartNanos, EndNanos);
 
   auto OmptEventInfo = reinterpret_cast<ompt::OmptEventInfoTy *>(Data);
   assert(OmptEventInfo && "Invalid OmptEventInfo");
   assert(OmptEventInfo->TraceRecord && "Invalid TraceRecord");
 
-  llvm::omp::target::ompt::RegionInterface.stopTargetSubmitTraceAsync(
-      OmptEventInfo->TraceRecord, OmptEventInfo->NumTeams, StartNanos,
-      EndNanos);
+  ompt::RegionInterface.stopTargetSubmitTraceAsync(OmptEventInfo->TraceRecord,
+                                                   OmptEventInfo->NumTeams,
+                                                   StartNanos, EndNanos);
 
   // Done processing, our responsibility to free the memory
   freeProfilerDataEntry(OmptEventInfo);
 }
 
-void llvm::omp::target::ompt::OmptProfilerTy::handleDataTransfer(
-    uint64_t StartNanos, uint64_t EndNanos, void *Data) {
+void ompt::OmptProfilerTy::handleDataTransfer(uint64_t StartNanos,
+                                              uint64_t EndNanos, void *Data) {
 
   if (!isProfilingEnabled())
     return;
@@ -125,23 +131,25 @@ void llvm::omp::target::ompt::OmptProfilerTy::handleDataTransfer(
   if (!Data)
     return;
 
+  DP("OMPT-Async: Time data for asynchronous execution: Start %lu "
+     "End %lu\n",
+     StartNanos, EndNanos);
+
   auto OmptEventInfo = reinterpret_cast<ompt::OmptEventInfoTy *>(Data);
   assert(OmptEventInfo && "Invalid OmptEventInfo");
   assert(OmptEventInfo->TraceRecord && "Invalid TraceRecord");
 
-  llvm::omp::target::ompt::RegionInterface.stopTargetDataMovementTraceAsync(
+  ompt::RegionInterface.stopTargetDataMovementTraceAsync(
       OmptEventInfo->TraceRecord, StartNanos, EndNanos);
 
   // Done processing, our responsibility to free the memory
   freeProfilerDataEntry(OmptEventInfo);
 }
 
-bool llvm::omp::target::ompt::OmptProfilerTy::isProfilingEnabled() {
-  return llvm::omp::target::ompt::TracingActive;
-}
+bool ompt::OmptProfilerTy::isProfilingEnabled() { return ompt::TracingActive; }
 
-void llvm::omp::target::ompt::OmptProfilerTy::setTimeConversionFactorsImpl(
-    double Slope, double Offset) {
+void ompt::OmptProfilerTy::setTimeConversionFactorsImpl(double Slope,
+                                                        double Offset) {
   DP("Using Time Slope: %f and Offset: %f \n", Slope, Offset);
   setOmptHostToDeviceRate(Slope, Offset);
 }

offload/plugins-nextgen/common/OMPT/OmptProfiler.h

@@ -19,7 +19,6 @@
 
 #include "OmptDeviceTracing.h"
 #include "OpenMP/OMPT/Callback.h"
-#include "OpenMP/OMPT/Interface.h"
 #include "Shared/Debug.h"
 #include "omp-tools.h"