[Comgr][OpenCL] Replace opencl-c.pch in favour of opencl-c-base.h + -fdeclare-opencl-builtins (#111)

Author: jmmartinez

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

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")

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()

amd/comgr/cmake/opencl_pch.cmake

@@ -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

amd/comgr/include/amd_comgr.h.in

@@ -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

amd/comgr/src/comgr-clang-command.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

amd/comgr/src/comgr-compiler.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>>

amd/comgr/src/comgr-device-libs.cpp

@@ -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();
 

amd/comgr/src/comgr-device-libs.h

@@ -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;

amd/comgr/src/comgr.cpp

@@ -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);
   }

amd/comgr/test-lit/comgr-sources/source-to-bc-with-dev-libs.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);
   }