[SYCL] Fix build options merge in program manager (#3849)

The program manager gets build options from several sources including
device code image, SYCL API and environment variables. Make sure that
options are space separated.

Author: vladimirlaz

sycl/source/detail/program_manager/program_manager.cpp

@@ -347,6 +347,54 @@ RT::PiProgram ProgramManager::createPIProgram(const RTDeviceBinaryImage &Img,
 
   return Res;
 }
+static void applyOptionsFromImage(std::string &CompileOpts,
+                                  std::string &LinkOpts,
+                                  const RTDeviceBinaryImage &Img) {
+  // Build options are overridden if environment variables are present.
+  // Environment variables are not changed during program lifecycle so it
+  // is reasonable to use static here to read them only once.
+  static const char *CompileOptsEnv =
+      SYCLConfig<SYCL_PROGRAM_COMPILE_OPTIONS>::get();
+  static const char *LinkOptsEnv = SYCLConfig<SYCL_PROGRAM_LINK_OPTIONS>::get();
+  // Update only if compile options are not overwritten by environment
+  // variable
+  if (!CompileOptsEnv) {
+    if (!CompileOpts.empty())
+      CompileOpts += " ";
+    CompileOpts += Img.getCompileOptions();
+  }
+
+  // The -vc-codegen option is always preserved for ESIMD kernels, regardless
+  // of the contents SYCL_PROGRAM_COMPILE_OPTIONS environment variable.
+  pi_device_binary_property isEsimdImage = Img.getProperty("isEsimdImage");
+  if (isEsimdImage && pi::DeviceBinaryProperty(isEsimdImage).asUint32()) {
+    if (!CompileOpts.empty())
+      CompileOpts += " ";
+    CompileOpts += "-vc-codegen";
+  }
+
+  // Update only if link options are not overwritten by environment variable
+  if (!LinkOptsEnv)
+    if (!LinkOpts.empty())
+      LinkOpts += " ";
+  LinkOpts += Img.getLinkOptions();
+}
+
+static void applyOptionsFromEnvironment(std::string &CompileOpts,
+                                        std::string &LinkOpts) {
+  // Build options are overridden if environment variables are present.
+  // Environment variables are not changed during program lifecycle so it
+  // is reasonable to use static here to read them only once.
+  static const char *CompileOptsEnv =
+      SYCLConfig<SYCL_PROGRAM_COMPILE_OPTIONS>::get();
+  if (CompileOptsEnv) {
+    CompileOpts = CompileOptsEnv;
+  }
+  static const char *LinkOptsEnv = SYCLConfig<SYCL_PROGRAM_LINK_OPTIONS>::get();
+  if (LinkOptsEnv) {
+    LinkOpts = LinkOptsEnv;
+  }
+}
 
 RT::PiProgram ProgramManager::getBuiltPIProgram(OSModuleHandle M,
                                                 const context &Context,
@@ -374,26 +422,12 @@ RT::PiProgram ProgramManager::getBuiltPIProgram(OSModuleHandle M,
 
   std::string CompileOpts;
   std::string LinkOpts;
-  // Build options are overridden if environment variables are present.
-  // Environment variables are not changed during program lifecycle so it
-  // is reasonable to use static here to read them only once.
-  static const char *CompileOptsEnv =
-      SYCLConfig<SYCL_PROGRAM_COMPILE_OPTIONS>::get();
-  if (CompileOptsEnv) {
-    CompileOpts = CompileOptsEnv;
-  } else { // Use build options only when the environment variable is missed
-    if (Prg) {
-      std::string BuildOptions = Prg->get_build_options();
-      if (!BuildOptions.empty()) {
-        CompileOpts += " ";
-        CompileOpts += BuildOptions;
-      }
-    }
-  }
-  static const char *LinkOptsEnv = SYCLConfig<SYCL_PROGRAM_LINK_OPTIONS>::get();
-  if (LinkOptsEnv) {
-    LinkOpts = LinkOptsEnv;
+  if (Prg) {
+    CompileOpts = Prg->get_build_options();
   }
+
+  applyOptionsFromEnvironment(CompileOpts, LinkOpts);
+
   SerializedObj SpecConsts;
   if (Prg)
     Prg->stableSerializeSpecConstRegistry(SpecConsts);
@@ -402,24 +436,8 @@ RT::PiProgram ProgramManager::getBuiltPIProgram(OSModuleHandle M,
                  &LinkOpts, &JITCompilationIsRequired, SpecConsts] {
     const RTDeviceBinaryImage &Img =
         getDeviceImage(M, KSId, Context, Device, JITCompilationIsRequired);
-    // Update only if compile options are not overwritten by environment
-    // variable
-    if (!CompileOptsEnv) {
-      CompileOpts += Img.getCompileOptions();
-    }
-
-    // The -vc-codegen option is always preserved for ESIMD kernels, regardless
-    // of the contents SYCL_PROGRAM_COMPILE_OPTIONS environment variable.
-    pi_device_binary_property isEsimdImage = Img.getProperty("isEsimdImage");
-    if (isEsimdImage && pi::DeviceBinaryProperty(isEsimdImage).asUint32()) {
-      if (!CompileOpts.empty())
-        CompileOpts += " ";
-      CompileOpts += "-vc-codegen";
-    }
 
-    // Update only if link options are not overwritten by environment variable
-    if (!LinkOptsEnv)
-      LinkOpts += Img.getLinkOptions();
+    applyOptionsFromImage(CompileOpts, LinkOpts, Img);
     ContextImplPtr ContextImpl = getSyclObjImpl(Context);
     const detail::plugin &Plugin = ContextImpl->getPlugin();
     RT::PiProgram NativePrg;
@@ -1518,41 +1536,15 @@ device_image_plain ProgramManager::build(const device_image_plain &DeviceImage,
 
   std::string CompileOpts;
   std::string LinkOpts;
-  // Build options are overridden if environment variables are present.
-  // Environment variables are not changed during program lifecycle so it
-  // is reasonable to use static here to read them only once.
-  static const char *CompileOptsEnv =
-      SYCLConfig<SYCL_PROGRAM_COMPILE_OPTIONS>::get();
-  if (CompileOptsEnv)
-    CompileOpts = CompileOptsEnv;
-
-  static const char *LinkOptsEnv = SYCLConfig<SYCL_PROGRAM_LINK_OPTIONS>::get();
-  if (LinkOptsEnv) {
-    LinkOpts = LinkOptsEnv;
-  }
+  applyOptionsFromEnvironment(CompileOpts, LinkOpts);
 
   const RTDeviceBinaryImage *ImgPtr = InputImpl->get_bin_image_ref();
   const RTDeviceBinaryImage &Img = *ImgPtr;
 
   // TODO: Unify this code with getBuiltPIProgram
   auto BuildF = [this, &Context, Img, &Devs, &CompileOpts, &LinkOpts,
                  &InputImpl] {
-    // Update only if compile options are not overwritten by environment
-    // variable
-    if (!CompileOptsEnv) {
-      CompileOpts += Img.getCompileOptions();
-      pi_device_binary_property isEsimdImage = Img.getProperty("isEsimdImage");
-
-      if (isEsimdImage && pi::DeviceBinaryProperty(isEsimdImage).asUint32()) {
-        if (!CompileOpts.empty())
-          CompileOpts += " ";
-        CompileOpts += "-vc-codegen";
-      }
-    }
-
-    // Update only if link options are not overwritten by environment variable
-    if (!LinkOptsEnv)
-      LinkOpts += Img.getLinkOptions();
+    applyOptionsFromImage(CompileOpts, LinkOpts, Img);
     ContextImplPtr ContextImpl = getSyclObjImpl(Context);
     const detail::plugin &Plugin = ContextImpl->getPlugin();
 

sycl/unittests/helpers/PiImage.hpp

@@ -51,8 +51,14 @@ class PiProperty {
 
 private:
   void updateNativeType() {
-    MNative = NativeType{const_cast<char *>(MName.c_str()),
-                         const_cast<char *>(MData.data()), MType, MData.size()};
+    if (MType == PI_PROPERTY_TYPE_UINT32) {
+      MNative = NativeType{const_cast<char *>(MName.c_str()), nullptr, MType,
+                           *((uint32_t *)MData.data())};
+    } else {
+      MNative =
+          NativeType{const_cast<char *>(MName.c_str()),
+                     const_cast<char *>(MData.data()), MType, MData.size()};
+    }
   }
   std::string MName;
   std::vector<char> MData;
@@ -366,6 +372,17 @@ void addSpecConstants(PiArray<PiProperty> SpecConstants,
                std::move(DefaultValues));
 }
 
+/// Utility function to add ESIMD kernel flag to property set.
+void addESIMDFlag(PiPropertySet &Props) {
+  std::vector<char> ValData(sizeof(uint32_t));
+  ValData[0] = 1;
+  PiProperty Prop{"isEsimdImage", ValData, PI_PROPERTY_TYPE_UINT32};
+
+  PiArray<PiProperty> Value{std::move(Prop)};
+
+  Props.insert(__SYCL_PI_PROPERTY_SET_SYCL_MISC_PROP, std::move(Value));
+}
+
 /// Utility function to generate offload entries for kernels without arguments.
 PiArray<PiOffloadEntry>
 makeEmptyKernels(std::initializer_list<std::string> KernelNames) {

sycl/unittests/misc/CMakeLists.txt

@@ -1,6 +1,7 @@
 set(sycl_lib_dir $<TARGET_FILE_DIR:sycl>)
 add_definitions(-DSYCL_LIB_DIR="${sycl_lib_dir}")
 add_sycl_unittest(MiscTests SHARED
-  OsUtils.cpp
   CircularBuffer.cpp
+  KernelBuildOptions.cpp
+  OsUtils.cpp
 )