Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][offload][OMPT] Cleanup of OMPT internals #109005

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

mhalk
Copy link
Contributor

@mhalk mhalk commented Sep 17, 2024

Removed OmptCallbacks.cpp since relevant contents were duplicated.
Because of the static linking there should be no change in functionality.

Removed OmptCallbacks.cpp since relevant contents were duplicated.
Because of the static linking there should be no change in functionality.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 17, 2024

@llvm/pr-subscribers-offload

Author: Michael Halkenhäuser (mhalk)

Changes

Removed OmptCallbacks.cpp since relevant contents were duplicated.
Because of the static linking there should be no change in functionality.


Full diff: https://github.com/llvm/llvm-project/pull/109005.diff

6 Files Affected:

  • (modified) offload/include/OpenMP/OMPT/Callback.h (+3-3)
  • (modified) offload/include/OpenMP/OMPT/Interface.h (+6-6)
  • (modified) offload/plugins-nextgen/common/CMakeLists.txt (-6)
  • (removed) offload/plugins-nextgen/common/OMPT/OmptCallback.cpp (-75)
  • (modified) offload/src/OpenMP/OMPT/Callback.cpp (+16-38)
  • (modified) offload/src/exports (-1)
diff --git a/offload/include/OpenMP/OMPT/Callback.h b/offload/include/OpenMP/OMPT/Callback.h
index 89c5731221e973..68cb43745eb1f8 100644
--- a/offload/include/OpenMP/OMPT/Callback.h
+++ b/offload/include/OpenMP/OMPT/Callback.h
@@ -11,8 +11,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef OMPTARGET_OPENMP_OMPT_CALLBACK_H
-#define OMPTARGET_OPENMP_OMPT_CALLBACK_H
+#ifndef OFFLOAD_INCLUDE_OPENMP_OMPT_CALLBACK_H
+#define OFFLOAD_INCLUDE_OPENMP_OMPT_CALLBACK_H
 
 #ifdef OMPT_SUPPORT
 
@@ -102,4 +102,4 @@ extern bool Initialized;
 #define performIfOmptInitialized(stmt)
 #endif // OMPT_SUPPORT
 
-#endif // OMPTARGET_OPENMP_OMPT_CALLBACK_H
+#endif // OFFLOAD_INCLUDE_OPENMP_OMPT_CALLBACK_H
diff --git a/offload/include/OpenMP/OMPT/Interface.h b/offload/include/OpenMP/OMPT/Interface.h
index 0dc1bad8f7ecea..43fb193bc75a6c 100644
--- a/offload/include/OpenMP/OMPT/Interface.h
+++ b/offload/include/OpenMP/OMPT/Interface.h
@@ -10,19 +10,19 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef _OMPTARGET_OMPTINTERFACE_H
-#define _OMPTARGET_OMPTINTERFACE_H
+#ifndef OFFLOAD_INCLUDE_OPENMP_OMPT_INTERFACE_H
+#define OFFLOAD_INCLUDE_OPENMP_OMPT_INTERFACE_H
 
 // Only provide functionality if target OMPT support is enabled
 #ifdef OMPT_SUPPORT
-#include <functional>
-#include <tuple>
-
 #include "Callback.h"
 #include "omp-tools.h"
 
 #include "llvm/Support/ErrorHandling.h"
 
+#include <functional>
+#include <tuple>
+
 #define OMPT_IF_BUILT(stmt) stmt
 
 /// Callbacks for target regions require task_data representing the
@@ -326,4 +326,4 @@ class ReturnAddressSetterRAII {
 #define OMPT_IF_BUILT(stmt)
 #endif
 
-#endif // _OMPTARGET_OMPTINTERFACE_H
+#endif // OFFLOAD_INCLUDE_OPENMP_OMPT_INTERFACE_H
diff --git a/offload/plugins-nextgen/common/CMakeLists.txt b/offload/plugins-nextgen/common/CMakeLists.txt
index 4dca5422087bba..fde4b2f930349e 100644
--- a/offload/plugins-nextgen/common/CMakeLists.txt
+++ b/offload/plugins-nextgen/common/CMakeLists.txt
@@ -38,12 +38,6 @@ elseif(${LIBOMPTARGET_GPU_LIBC_SUPPORT})
   endif()
 endif()
 
-# If we have OMPT enabled include it in the list of sources.
-if (OMPT_TARGET_DEFAULT AND LIBOMPTARGET_OMPT_SUPPORT)
-  target_sources(PluginCommon PRIVATE OMPT/OmptCallback.cpp)
-  target_include_directories(PluginCommon PRIVATE OMPT)
-endif()
-
 # Define the TARGET_NAME and DEBUG_PREFIX.
 target_compile_definitions(PluginCommon PRIVATE
   TARGET_NAME="PluginInterface"
diff --git a/offload/plugins-nextgen/common/OMPT/OmptCallback.cpp b/offload/plugins-nextgen/common/OMPT/OmptCallback.cpp
deleted file mode 100644
index fb8a156fe5767a..00000000000000
--- a/offload/plugins-nextgen/common/OMPT/OmptCallback.cpp
+++ /dev/null
@@ -1,75 +0,0 @@
-//===---------- OmptCallback.cpp - Generic OMPT callbacks --------- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// OMPT support for PluginInterface
-//
-//===----------------------------------------------------------------------===//
-
-#ifdef OMPT_SUPPORT
-
-#include "llvm/Support/DynamicLibrary.h"
-
-#include <cstdlib>
-#include <cstring>
-#include <memory>
-
-#include "Shared/Debug.h"
-
-#include "OpenMP/OMPT/Callback.h"
-#include "OpenMP/OMPT/Connector.h"
-
-using namespace llvm::omp::target::ompt;
-
-bool llvm::omp::target::ompt::Initialized = false;
-
-ompt_get_callback_t llvm::omp::target::ompt::lookupCallbackByCode = nullptr;
-ompt_function_lookup_t llvm::omp::target::ompt::lookupCallbackByName = nullptr;
-
-int llvm::omp::target::ompt::initializeLibrary(ompt_function_lookup_t lookup,
-                                               int initial_device_num,
-                                               ompt_data_t *tool_data) {
-  DP("OMPT: Executing initializeLibrary (libomptarget)\n");
-#define bindOmptFunctionName(OmptFunction, DestinationFunction)                \
-  if (lookup)                                                                  \
-    DestinationFunction = (OmptFunction##_t)lookup(#OmptFunction);             \
-  DP("OMPT: initializeLibrary (libomptarget) bound %s=%p\n",                   \
-     #DestinationFunction, ((void *)(uint64_t)DestinationFunction));
-
-  bindOmptFunctionName(ompt_get_callback, lookupCallbackByCode);
-#undef bindOmptFunctionName
-
-  // Store pointer of 'ompt_libomp_target_fn_lookup' for use by the plugin
-  lookupCallbackByName = lookup;
-
-  Initialized = true;
-
-  return 0;
-}
-
-void llvm::omp::target::ompt::finalizeLibrary(ompt_data_t *tool_data) {
-  DP("OMPT: Executing finalizeLibrary (libomptarget)\n");
-}
-
-void llvm::omp::target::ompt::connectLibrary() {
-  DP("OMPT: Entering connectLibrary (libomptarget)\n");
-  /// Connect plugin instance with libomptarget
-  OmptLibraryConnectorTy LibomptargetConnector("libomptarget");
-  ompt_start_tool_result_t OmptResult;
-
-  // Initialize OmptResult with the init and fini functions that will be
-  // called by the connector
-  OmptResult.initialize = ompt::initializeLibrary;
-  OmptResult.finalize = ompt::finalizeLibrary;
-  OmptResult.tool_data.value = 0;
-
-  // Now call connect that causes the above init/fini functions to be called
-  LibomptargetConnector.connect(&OmptResult);
-  DP("OMPT: Exiting connectLibrary (libomptarget)\n");
-}
-
-#endif
diff --git a/offload/src/OpenMP/OMPT/Callback.cpp b/offload/src/OpenMP/OMPT/Callback.cpp
index f2964281eeb95f..ab0942ed4fd3f7 100644
--- a/offload/src/OpenMP/OMPT/Callback.cpp
+++ b/offload/src/OpenMP/OMPT/Callback.cpp
@@ -10,14 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef OMPT_SUPPORT
-
-extern "C" {
-/// Dummy definition when OMPT is disabled
-void ompt_libomptarget_connect() {}
-}
-
-#else // OMPT_SUPPORT is set
+#ifdef OMPT_SUPPORT
 
 #include <cstdlib>
 #include <cstring>
@@ -34,8 +27,6 @@ void ompt_libomptarget_connect() {}
 #undef DEBUG_PREFIX
 #define DEBUG_PREFIX "OMPT"
 
-using namespace llvm::omp::target::ompt;
-
 // Define OMPT callback functions (bound to actual callbacks later on)
 #define defineOmptCallback(Name, Type, Code)                                   \
   Name##_t llvm::omp::target::ompt::Name##_fn = nullptr;
@@ -43,6 +34,8 @@ FOREACH_OMPT_NOEMI_EVENT(defineOmptCallback)
 FOREACH_OMPT_EMI_EVENT(defineOmptCallback)
 #undef defineOmptCallback
 
+using namespace llvm::omp::target::ompt;
+
 /// Forward declaration
 class LibomptargetRtlFinalizer;
 
@@ -226,26 +219,26 @@ void Interface::endTargetDataRetrieve(int64_t SrcDeviceId, void *SrcPtrBegin,
   endTargetDataOperation();
 }
 
-void Interface::beginTargetSubmit(unsigned int numTeams) {
+void Interface::beginTargetSubmit(unsigned int NumTeams) {
   if (ompt_callback_target_submit_emi_fn) {
     // HostOpId is set by the tool. Invoke the tool supplied target submit EMI
     // callback
     ompt_callback_target_submit_emi_fn(ompt_scope_begin, &TargetData, &HostOpId,
-                                       numTeams);
+                                       NumTeams);
   } else if (ompt_callback_target_submit_fn) {
     // HostOpId is set by the runtime
     HostOpId = createOpId();
-    ompt_callback_target_submit_fn(TargetData.value, HostOpId, numTeams);
+    ompt_callback_target_submit_fn(TargetData.value, HostOpId, NumTeams);
   }
 }
 
-void Interface::endTargetSubmit(unsigned int numTeams) {
+void Interface::endTargetSubmit(unsigned int NumTeams) {
   // Only EMI callback handles end scope
   if (ompt_callback_target_submit_emi_fn) {
     // HostOpId is set by the tool. Invoke the tool supplied target submit EMI
     // callback
     ompt_callback_target_submit_emi_fn(ompt_scope_end, &TargetData, &HostOpId,
-                                       numTeams);
+                                       NumTeams);
   }
 }
 
@@ -458,7 +451,7 @@ class LibomptargetRtlFinalizer {
 
   void finalize() {
     for (auto FinalizationFunction : RtlFinalizationFunctions)
-      FinalizationFunction(/* tool_data */ nullptr);
+      FinalizationFunction(/*tool_data=*/nullptr);
     RtlFinalizationFunctions.clear();
   }
 
@@ -469,10 +462,11 @@ class LibomptargetRtlFinalizer {
 int llvm::omp::target::ompt::initializeLibrary(ompt_function_lookup_t lookup,
                                                int initial_device_num,
                                                ompt_data_t *tool_data) {
-  DP("Executing initializeLibrary (libomp)\n");
+  DP("Executing initializeLibrary\n");
 #define bindOmptFunctionName(OmptFunction, DestinationFunction)                \
-  DestinationFunction = (OmptFunction##_t)lookup(#OmptFunction);               \
-  DP("initializeLibrary (libomp) bound %s=%p\n", #DestinationFunction,         \
+  if (lookup)                                                                  \
+    DestinationFunction = (OmptFunction##_t)lookup(#OmptFunction);             \
+  DP("initializeLibrary bound %s=%p\n", #DestinationFunction,                  \
      ((void *)(uint64_t)DestinationFunction));
 
   bindOmptFunctionName(ompt_get_callback, lookupCallbackByCode);
@@ -499,7 +493,7 @@ int llvm::omp::target::ompt::initializeLibrary(ompt_function_lookup_t lookup,
 }
 
 void llvm::omp::target::ompt::finalizeLibrary(ompt_data_t *data) {
-  DP("Executing finalizeLibrary (libomp)\n");
+  DP("Executing finalizeLibrary\n");
   // Before disabling OMPT, call the (plugin) finalizations that were registered
   // with this library
   LibraryFinalizer->finalize();
@@ -508,7 +502,7 @@ void llvm::omp::target::ompt::finalizeLibrary(ompt_data_t *data) {
 }
 
 void llvm::omp::target::ompt::connectLibrary() {
-  DP("Entering connectLibrary (libomp)\n");
+  DP("Entering connectLibrary\n");
   // Connect with libomp
   static OmptLibraryConnectorTy LibompConnector("libomp");
   static ompt_start_tool_result_t OmptResult;
@@ -531,23 +525,7 @@ void llvm::omp::target::ompt::connectLibrary() {
   FOREACH_OMPT_EMI_EVENT(bindOmptCallback)
 #undef bindOmptCallback
 
-  DP("Exiting connectLibrary (libomp)\n");
+  DP("Exiting connectLibrary\n");
 }
 
-extern "C" {
-/// Used for connecting libomptarget with a plugin
-void ompt_libomptarget_connect(ompt_start_tool_result_t *result) {
-  DP("Enter ompt_libomptarget_connect\n");
-  if (Initialized && result && LibraryFinalizer) {
-    // Cache each fini function, so that they can be invoked on exit
-    LibraryFinalizer->registerRtl(result->finalize);
-    // Invoke the provided init function with the lookup function maintained
-    // in this library so that callbacks maintained by this library are
-    // retrieved.
-    result->initialize(lookupCallbackByName,
-                       /* initial_device_num */ 0, /* tool_data */ nullptr);
-  }
-  DP("Leave ompt_libomptarget_connect\n");
-}
-}
 #endif // OMPT_SUPPORT
diff --git a/offload/src/exports b/offload/src/exports
index 7bdc7d2a531bb3..2406776c1fb5fc 100644
--- a/offload/src/exports
+++ b/offload/src/exports
@@ -70,7 +70,6 @@ VERS1.0 {
     __tgt_interop_init;
     __tgt_interop_use;
     __tgt_interop_destroy;
-    ompt_libomptarget_connect;
     __llvmPushCallConfiguration;
     __llvmPopCallConfiguration;
     llvmLaunchKernel;

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, nice to see this stuff moved out of the plugins now that they're not separate programs.

Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.
Gave it a quick run through a buildbot recreation w/o issues.

Copy link
Contributor

@dhruvachak dhruvachak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhalk mhalk merged commit d36f66b into llvm:main Sep 23, 2024
8 checks passed
@mhalk mhalk deleted the mhalk/fix/ompt_refactor_static_lib branch September 23, 2024 09:58
@mhalk
Copy link
Contributor Author

mhalk commented Sep 23, 2024

Thanks for taking the time to review!

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Sep 23, 2024
lands and reverts internally
  d36f66b [NFC][offload][OMPT] Cleanup of OMPT internals (llvm#109005)
downstream gerrit review contains the content we want landed.

Change-Id: I9aebe6a0aa1775d85f7225642bcb6bdc6c5de41f
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Sep 23, 2024
Removed OmptCallbacks.cpp since relevant contents were duplicated.
Because of the static linking there should be no change in functionality.

This commit is meant to replace the following upstream PR:
llvm#109005

Change-Id: Iddc56ae559f7387056bdcf2d6b8860ab9fc5956a
augusto2112 pushed a commit to augusto2112/llvm-project that referenced this pull request Sep 26, 2024
Removed `OmptCallbacks.cpp` since relevant contents were duplicated.
Because of the static linking there should be no change in
functionality.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
Removed `OmptCallbacks.cpp` since relevant contents were duplicated.
Because of the static linking there should be no change in
functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants