[Comgr][OpenCL] Remove opencl-c.pch in favour of opencl-c-base.h +

jmmartinez · jmmartinez · commit b754bb10cdfe · 2025-04-23T13:16:17.000+02:00
-fdeclare-opencl-builtins Comgr does not embed anymore the precompiled versions of opencl-c, however, it still has to embed the source version of opencl-c-base (on some builds we do not ship Clang's resource-dir). The AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS is kept around for compatibility, but it doesn't add the headers anymore to the output set. If used, the input set is forwarded to the output set unchanged. Depens on llvm#134598 to pass a full OpenCL Conformance test.
diff --git a/amd/comgr/CMakeLists.txt b/amd/comgr/CMakeLists.txt
@@ -229,7 +229,7 @@ endif()
 list(APPEND AMD_COMGR_PRIVATE_COMPILE_DEFINITIONS AMD_COMGR_EXPORT)
 
 include(bc2h)
-include(opencl_pch)
+include(opencl_header)
 include(DeviceLibs)
 
 # Add major version to the name on windows, including Win64
diff --git a/amd/comgr/cmake/DeviceLibs.cmake b/amd/comgr/cmake/DeviceLibs.cmake
@@ -65,20 +65,17 @@ endforeach()
 list(JOIN TARGETS_INCLUDES "\n" TARGETS_INCLUDES)
 file(GENERATE OUTPUT ${GEN_LIBRARY_INC_FILE} CONTENT "${TARGETS_INCLUDES}")
 
-foreach(OPENCL_VERSION 1.2 2.0)
-  string(REPLACE . _ OPENCL_UNDERSCORE_VERSION ${OPENCL_VERSION})
-  add_custom_command(OUTPUT ${INC_DIR}/opencl${OPENCL_VERSION}-c.inc
-    COMMAND bc2h ${CMAKE_CURRENT_BINARY_DIR}/opencl${OPENCL_VERSION}-c.pch
-                 ${INC_DIR}/opencl${OPENCL_VERSION}-c.inc
-                 opencl${OPENCL_UNDERSCORE_VERSION}_c
-    DEPENDS bc2h ${CMAKE_CURRENT_BINARY_DIR}/opencl${OPENCL_VERSION}-c.pch
-    COMMENT "Generating opencl${OPENCL_VERSION}-c.inc"
-  )
-  set_property(DIRECTORY APPEND PROPERTY
-    ADDITIONAL_MAKE_CLEAN_FILES ${INC_DIR}/opencl${OPENCL_VERSION}-c.inc)
-  add_custom_target(opencl${OPENCL_VERSION}-c.inc_target DEPENDS ${INC_DIR}/opencl${OPENCL_VERSION}-c.inc)
-  add_dependencies(amd_comgr opencl${OPENCL_VERSION}-c.inc_target)
-endforeach()
+add_custom_command(OUTPUT ${INC_DIR}/opencl-c-base.inc
+  COMMAND bc2h ${OPENCL_C_H}
+                ${INC_DIR}/opencl-c-base.inc
+                opencl_c_base
+  DEPENDS bc2h clang ${OPENCL_C_H}
+  COMMENT "Generating opencl-c-base.inc"
+)
+set_property(DIRECTORY APPEND PROPERTY
+  ADDITIONAL_MAKE_CLEAN_FILES ${INC_DIR}/opencl-c-base.inc)
+add_custom_target(opencl-c-base.inc_target DEPENDS ${INC_DIR}/opencl-c-base.inc)
+add_dependencies(amd_comgr opencl-c-base.inc_target)
 
 set(TARGETS_DEFS "")
 list(APPEND TARGETS_DEFS "#ifndef AMD_DEVICE_LIBS_TARGET\n#define AMD_DEVICE_LIBS_TARGET(t)\n#endif")
diff --git a/amd/comgr/cmake/opencl_header.cmake b/amd/comgr/cmake/opencl_header.cmake
@@ -0,0 +1,24 @@
+if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
+  find_package(Clang REQUIRED CONFIG)
+
+  # FIXME: CLANG_CMAKE_DIR seems like the most stable way to find this, but
+  # really there is no way to reliably discover this header.
+  #
+  # We effectively back up to the Clang output directory (for the case of a build
+  # tree) or install prefix (for the case of an installed copy), and then search
+  # for a file named opencl-c-base.h anywhere below that. We take the first result in
+  # the case where there are multiple (e.g. if there is an installed copy nested
+  # in a build directory). This is a bit imprecise, but it covers cases like MSVC
+  # adding some additional configuration-specific subdirectories to the build
+  # tree but not to an installed copy.
+  file(GLOB_RECURSE OPENCL_C_H_LIST "${CLANG_CMAKE_DIR}/../../../*/opencl-c-base.h")
+
+  list(GET OPENCL_C_H_LIST 0 OPENCL_C_H)
+
+  if (NOT EXISTS "${OPENCL_C_H}" OR IS_DIRECTORY "${OPENCL_C_H}")
+    message(FATAL_ERROR "Unable to locate opencl-c-base.h from the supplied Clang. The path '${CLANG_CMAKE_DIR}/../../../*' was searched.")
+  endif()
+else()
+  get_target_property(clang_build_header_dir clang-resource-headers RUNTIME_OUTPUT_DIRECTORY)
+  set(OPENCL_C_H "${clang_build_header_dir}/opencl-c-base.h")
+endif()
diff --git a/amd/comgr/cmake/opencl_pch.cmake b/amd/comgr/cmake/opencl_pch.cmake
diff --git a/amd/comgr/include/amd_comgr.h.in b/amd/comgr/include/amd_comgr.h.in
@@ -1612,17 +1612,18 @@ typedef enum amd_comgr_action_kind_s {
    */
   AMD_COMGR_ACTION_SOURCE_TO_PREPROCESSOR = 0x0,
   /**
-   * Copy all existing data objects in @p input to @p output, then add the
-   * device-specific and language-specific precompiled headers required for
-   * compilation.
+   * Copy all existing data objects in @p input to @p output.
    *
-   * Currently the only supported languages are @p AMD_COMGR_LANGUAGE_OPENCL_1_2
-   * and @p AMD_COMGR_LANGUAGE_OPENCL_2_0.
+   * Currently the action is a no-op, as the OpenCL pre-compiled headers
+   * are no longer used.
    *
-   * Return @p AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT if isa name or language
-   * is not set in @p info, or the language is not supported.
+   * Return @p AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT if any of the
+   * input or output are not initialized.
    */
-  AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS = 0x1,
+  AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS
+    AMD_COMGR_DEPRECATED("Will be removed in Comgr v4.0. Currently the action\
+    is a no-op, as the OpenCL pre-compiled headers are no longer used.")
+    = 0x1,
   /**
    * Compile each source data object in @p input in order. For each
    * successful compilation add a bc data object to @p result. Resolve
diff --git a/amd/comgr/src/comgr-clang-command.cpp b/amd/comgr/src/comgr-clang-command.cpp
@@ -54,9 +54,11 @@ bool skipProblematicFlag(IteratorTy &It, const IteratorTy &End) {
   // Skip include paths, these should have been handled by preprocessing the
   // source first. Sadly, these are passed also to the middle-end commands. Skip
   // debug related flags (they should be ignored) like -dumpdir (used for
-  // profiling/coverage/split-dwarf)
+  // profiling/coverage/split-dwarf).
+  // Skip flags related to opencl-c headers or device-libs builtins.
   StringRef Arg = *It;
-  static const StringSet<> FlagsWithPathArg = {"-I", "-dumpdir"};
+  static const StringSet<> FlagsWithPathArg = {"-I", "-dumpdir", "-include",
+                                               "-mlink-builtin-bitcode"};
   bool IsFlagWithPathArg = It + 1 != End && FlagsWithPathArg.contains(Arg);
   if (IsFlagWithPathArg) {
     ++It;
@@ -119,19 +121,6 @@ void ClangCommand::addOptionsIdentifier(HashAlgorithm &H) const {
       continue;
 
     StringRef Arg = *It;
-    static const StringSet<> FlagsWithFileArgEmbededInComgr = {
-        "-include-pch", "-mlink-builtin-bitcode"};
-    if (FlagsWithFileArgEmbededInComgr.contains(Arg)) {
-      // The next argument is a path to a "secondary" input-file (pre-compiled
-      // header or device-libs builtin)
-      // These two files kinds of files are embedded in comgr at compile time,
-      // and in normally their remain constant with comgr's build. The user is
-      // not able to change them.
-      ++It;
-      if (It == End)
-        break;
-      continue;
-    }
 
     // input files are considered by their content
     // output files should not be considered at all
diff --git a/amd/comgr/src/comgr-compiler.cpp b/amd/comgr/src/comgr-compiler.cpp
@@ -311,6 +311,14 @@ bool AssemblerInvocation::createFromArgs(AssemblerInvocation &Opts,
 }
 
 namespace {
+bool needsPreprocessing(DataObject *O) {
+  if (O->DataKind != AMD_COMGR_DATA_KIND_SOURCE)
+    return false;
+  StringRef Ext = path::extension(O->Name);
+  bool IsPreprocessedSource = Ext == ".i";
+  return !IsPreprocessedSource;
+}
+
 std::unique_ptr<raw_fd_ostream> getOutputStream(AssemblerInvocation &Opts,
                                                 DiagnosticsEngine &Diags,
                                                 bool Binary) {
@@ -941,7 +949,10 @@ AMDGPUCompiler::processFiles(amd_comgr_data_kind_t OutputKind,
     ScopedDataObjectReleaser SDOR(OutputT);
 
     DataObject *Output = DataObject::convert(OutputT);
-    Output->setName(std::string(Input->Name) + OutputSuffix);
+
+    SmallString<128> OutputName(Input->Name);
+    sys::path::replace_extension(OutputName, OutputSuffix);
+    Output->setName(OutputName);
 
     auto OutputFilePath = getFilePath(Output, OutputDir);
 
@@ -963,6 +974,28 @@ AMDGPUCompiler::processFiles(amd_comgr_data_kind_t OutputKind,
 }
 
 amd_comgr_status_t AMDGPUCompiler::addIncludeFlags() {
+  if (none_of(InSet->DataObjects, needsPreprocessing))
+    return AMD_COMGR_STATUS_SUCCESS;
+
+  switch (ActionInfo->Language) {
+  case AMD_COMGR_LANGUAGE_OPENCL_1_2:
+  case AMD_COMGR_LANGUAGE_OPENCL_2_0: {
+    SmallString<128> OpenCLCBasePath = IncludeDir;
+    sys::path::append(OpenCLCBasePath, "opencl-c-base.h");
+    if (auto Status =
+            outputToFile(getOpenCLCBaseHeaderContents(), OpenCLCBasePath)) {
+      return Status;
+    }
+    Args.push_back("-include");
+    Args.push_back(Saver.save(OpenCLCBasePath.c_str()).data());
+    Args.push_back("-Xclang");
+    Args.push_back("-fdeclare-opencl-builtins");
+    break;
+  }
+  default:
+    break;
+  }
+
   if (ActionInfo->Path) {
     Args.push_back("-I");
     Args.push_back(ActionInfo->Path);
@@ -1023,22 +1056,24 @@ amd_comgr_status_t AMDGPUCompiler::addCompilationFlags() {
 
   Args.push_back("-x");
 
+  bool NeedsPreprocessing = any_of(InSet->DataObjects, needsPreprocessing);
+
   switch (ActionInfo->Language) {
   case AMD_COMGR_LANGUAGE_LLVM_IR:
     Args.push_back("ir");
     break;
   case AMD_COMGR_LANGUAGE_OPENCL_1_2:
-    Args.push_back("cl");
+    Args.push_back(NeedsPreprocessing ? "cl" : "cl-cpp-output");
     Args.push_back("-std=cl1.2");
     Args.push_back("-cl-no-stdinc");
     break;
   case AMD_COMGR_LANGUAGE_OPENCL_2_0:
-    Args.push_back("cl");
+    Args.push_back(NeedsPreprocessing ? "cl" : "cl-cpp-output");
     Args.push_back("-std=cl2.0");
     Args.push_back("-cl-no-stdinc");
     break;
   case AMD_COMGR_LANGUAGE_HIP:
-    Args.push_back("hip");
+    Args.push_back(NeedsPreprocessing ? "hip" : "hip-cpp-output");
     Args.push_back("--offload-device-only");
     // Pass a cuid that depends on the input files
     // Otherwise, a random (which depends on the /tmp/comgr-xxxxx path) cuid is
diff --git a/amd/comgr/src/comgr-device-libs.cpp b/amd/comgr/src/comgr-device-libs.cpp
@@ -25,45 +25,18 @@ using namespace llvm;
 namespace COMGR {
 
 namespace {
-amd_comgr_status_t addObject(DataSet *DataSet, amd_comgr_data_kind_t Kind,
-                             const char *Name, const void *Data, size_t Size) {
-  DataObject *Obj = DataObject::allocate(Kind);
-  if (!Obj) {
-    return AMD_COMGR_STATUS_ERROR_OUT_OF_RESOURCES;
-  }
-  if (auto Status = Obj->setName(Name)) {
-    return Status;
-  }
-  if (auto Status =
-          Obj->setData(StringRef(reinterpret_cast<const char *>(Data), Size))) {
-    return Status;
-  }
-  DataSet->DataObjects.insert(Obj);
-  return AMD_COMGR_STATUS_SUCCESS;
-}
-
 #include "libraries.inc"
 #include "libraries_sha.inc"
-#include "opencl1.2-c.inc"
-#include "opencl2.0-c.inc"
+#include "opencl-c-base.inc"
 } // namespace
 
 ArrayRef<unsigned char> getDeviceLibrariesIdentifier() {
   return DEVICE_LIBS_ID;
 }
 
-amd_comgr_status_t addPrecompiledHeaders(DataAction *ActionInfo,
-                                         DataSet *ResultSet) {
-  switch (ActionInfo->Language) {
-  case AMD_COMGR_LANGUAGE_OPENCL_1_2:
-    return addObject(ResultSet, AMD_COMGR_DATA_KIND_PRECOMPILED_HEADER,
-                     "opencl1.2-c.pch", opencl1_2_c, opencl1_2_c_size);
-  case AMD_COMGR_LANGUAGE_OPENCL_2_0:
-    return addObject(ResultSet, AMD_COMGR_DATA_KIND_PRECOMPILED_HEADER,
-                     "opencl2.0-c.pch", opencl2_0_c, opencl2_0_c_size);
-  default:
-    return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;
-  }
+StringRef getOpenCLCBaseHeaderContents() {
+  return StringRef(reinterpret_cast<const char *>(opencl_c_base),
+                   opencl_c_base_size);
 }
 
 llvm::ArrayRef<std::tuple<llvm::StringRef, llvm::StringRef>>
diff --git a/amd/comgr/src/comgr-device-libs.h b/amd/comgr/src/comgr-device-libs.h
@@ -9,7 +9,6 @@
 #ifndef COMGR_DEVICE_LIBS_H
 #define COMGR_DEVICE_LIBS_H
 
-#include "amd_comgr.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include <tuple>
@@ -19,11 +18,8 @@ namespace COMGR {
 struct DataAction;
 struct DataSet;
 
-amd_comgr_status_t addPrecompiledHeaders(DataAction *ActionInfo,
-                                         DataSet *ResultSet);
-
 llvm::ArrayRef<unsigned char> getDeviceLibrariesIdentifier();
-
+llvm::StringRef getOpenCLCBaseHeaderContents();
 llvm::ArrayRef<std::tuple<llvm::StringRef, llvm::StringRef>>
 getDeviceLibraries();
 
diff --git a/amd/comgr/src/comgr.cpp b/amd/comgr/src/comgr.cpp
@@ -176,21 +176,6 @@ amd_comgr_status_t dispatchCompilerAction(amd_comgr_action_kind_t ActionKind,
   }
 }
 
-amd_comgr_status_t dispatchAddAction(amd_comgr_action_kind_t ActionKind,
-                                     DataAction *ActionInfo, DataSet *InputSet,
-                                     DataSet *ResultSet) {
-  for (DataObject *Data : InputSet->DataObjects) {
-    Data->RefCount++;
-    ResultSet->DataObjects.insert(Data);
-  }
-  switch (ActionKind) {
-  case AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS:
-    return addPrecompiledHeaders(ActionInfo, ResultSet);
-  default:
-    return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;
-  }
-}
-
 StringRef getLanguageName(amd_comgr_language_t Language) {
   switch (Language) {
   case AMD_COMGR_LANGUAGE_NONE:
@@ -1381,8 +1366,13 @@ amd_comgr_status_t AMD_COMGR_API
                                             ResultSetP, *LogP);
       break;
     case AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS:
-      ActionStatus =
-          dispatchAddAction(ActionKind, ActionInfoP, InputSetP, ResultSetP);
+      // Redirect the input to the output.
+      // Deprecate and remove this action.
+      for (DataObject *Data : InputSetP->DataObjects) {
+        Data->RefCount++;
+        ResultSetP->DataObjects.insert(Data);
+      }
+      ActionStatus = AMD_COMGR_STATUS_SUCCESS;
       break;
     default:
       ActionStatus = AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;
diff --git a/amd/comgr/test-lit/comgr-sources/source-to-bc-with-dev-libs.c b/amd/comgr/test-lit/comgr-sources/source-to-bc-with-dev-libs.c
@@ -55,9 +55,9 @@ int main(int argc, char *argv[]) {
   amd_comgr_(action_data_count(DataSetPch,
                                AMD_COMGR_DATA_KIND_PRECOMPILED_HEADER, &Count));
 
-  if (Count != 1) {
+  if (Count != 0) {
     printf("AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS Failed: "
-           "produced %zu precompiled header objects (expected 1)\n",
+           "produced %zu precompiled header objects (expected 0)\n",
            Count);
     exit(1);
   }
diff --git a/amd/comgr/test/compile_source_with_device_libs_to_bc_test.c b/amd/comgr/test/compile_source_with_device_libs_to_bc_test.c
@@ -60,9 +60,9 @@ int main(int argc, char *argv[]) {
       DataSetPch, AMD_COMGR_DATA_KIND_PRECOMPILED_HEADER, &Count);
   checkError(Status, "amd_comgr_action_data_count");
 
-  if (Count != 1) {
+  if (Count != 0) {
     printf("AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS Failed: "
-           "produced %zu precompiled header objects (expected 1)\n",
+           "produced %zu precompiled header objects (expected 0)\n",
            Count);
     exit(1);
   }
diff --git a/amd/comgr/test/compile_source_with_device_libs_to_bc_with_vfs_test.c b/amd/comgr/test/compile_source_with_device_libs_to_bc_with_vfs_test.c
