apply suggested changes

Author: HabKaffee

sycl/source/detail/program_manager/program_manager.cpp

@@ -366,59 +366,50 @@ RT::PiProgram ProgramManager::createPIProgram(const RTDeviceBinaryImage &Img,
   return Res;
 }
 
-static void addEsimdImageCompileOptions(std::string &CompileOpts) {
-  if (!CompileOpts.empty())
-    CompileOpts += " ";
-  CompileOpts += "-vc-codegen";
-}
-
-static void applyLinkOptionsFromImage(std::string &LinkOpts,
-                                      const RTDeviceBinaryImage &Img) {
-  if (!LinkOpts.empty())
-    LinkOpts += " ";
-  const char *TemporaryStr = Img.getLinkOptions();
-  if (TemporaryStr != nullptr)
-    LinkOpts += std::string(TemporaryStr);
-}
-
-static void applyCompileOptionsFromImage(
-    std::string &CompileOpts, const RTDeviceBinaryImage &Img,
-    const pi_device_binary_property &isEsimdImage = nullptr) {
-  if (!CompileOpts.empty())
-    CompileOpts += " ";
-  const char *TemporaryStr = Img.getCompileOptions();
-  if (TemporaryStr != nullptr)
-    CompileOpts += TemporaryStr;
-  if (isEsimdImage) {
-    addEsimdImageCompileOptions(CompileOpts);
+static void appendLinkOptionsFromImage(std::string &LinkOpts,
+                                       const RTDeviceBinaryImage &Img) {
+  static const char *LinkOptsEnv = SYCLConfig<SYCL_PROGRAM_LINK_OPTIONS>::get();
+  // Update only if link options are not overwritten by environment variable
+  if (!LinkOptsEnv) {
+    if (!LinkOpts.empty())
+      LinkOpts += " ";
+    const char *TemporaryStr = Img.getLinkOptions();
+    if (TemporaryStr != nullptr)
+      LinkOpts += std::string(TemporaryStr);
   }
 }
 
-static void applyOptionsFromImage(std::string &CompileOpts,
-                                  std::string &LinkOpts,
-                                  const RTDeviceBinaryImage &Img) {
+static void appendCompileOptionsFromImage(std::string &CompileOpts,
+                                          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();
+  pi_device_binary_property isEsimdImage = Img.getProperty("isEsimdImage");
   // Update only if compile options are not overwritten by environment
   // variable
+  if (!CompileOptsEnv) {
+    if (!CompileOpts.empty())
+      CompileOpts += " ";
+    const char *TemporaryStr = Img.getCompileOptions();
+    if (TemporaryStr != nullptr)
+      CompileOpts += std::string(TemporaryStr);
+  }
   // 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 (!CompileOptsEnv) {
-    applyCompileOptionsFromImage(CompileOpts, Img, isEsimdImage);
-  } else if (isEsimdImage &&
-             pi::DeviceBinaryProperty(isEsimdImage).asUint32()) {
-    addEsimdImageCompileOptions(CompileOpts);
+  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) {
-    applyLinkOptionsFromImage(LinkOpts, Img);
-  }
+static void applyOptionsFromImage(std::string &CompileOpts,
+                                  std::string &LinkOpts,
+                                  const RTDeviceBinaryImage &Img) {
+  appendCompileOptionsFromImage(CompileOpts, Img);
+  appendLinkOptionsFromImage(LinkOpts, Img);
 }
 
 static void applyOptionsFromEnvironment(std::string &CompileOpts,
@@ -1004,10 +995,9 @@ ProgramManager::ProgramPtr ProgramManager::build(
 
   const detail::plugin &Plugin = Context->getPlugin();
   if (LinkPrograms.empty() && !ForceLink) {
-    std::string Options = CompileOptions;
-    if (!LinkOptions.empty()) {
-      Options += " " + LinkOptions;
-    }
+    std::string Options = LinkOptions.empty()
+                              ? CompileOptions
+                              : (CompileOptions + " " + LinkOptions);
     RT::PiResult Error = Plugin.call_nocheck<PiApiKind::piProgramBuild>(
         Program.get(), /*num devices =*/1, &Device, Options.c_str(), nullptr,
         nullptr);
@@ -1693,12 +1683,12 @@ ProgramManager::compile(const device_image_plain &DeviceImage,
   // TODO: Set spec constatns here.
 
   // TODO: Handle zero sized Device list.
-
-  const char *compileOptions =
-      InputImpl->get_bin_image_ref()->getCompileOptions();
+  std::string CompileOptions;
+  appendCompileOptionsFromImage(CompileOptions,
+                                *(InputImpl->get_bin_image_ref()));
   RT::PiResult Error = Plugin.call_nocheck<PiApiKind::piProgramCompile>(
       ObjectImpl->get_program_ref(), /*num devices=*/Devs.size(),
-      PIDevices.data(), compileOptions,
+      PIDevices.data(), CompileOptions.c_str(),
       /*num_input_headers=*/0, /*input_headers=*/nullptr,
       /*header_include_names=*/nullptr,
       /*pfn_notify=*/nullptr, /*user_data*/ nullptr);
@@ -1731,8 +1721,8 @@ ProgramManager::link(const std::vector<device_image_plain> &DeviceImages,
   for (const device_image_plain &DeviceImage : DeviceImages) {
     const std::shared_ptr<device_image_impl> &InputImpl =
         getSyclObjImpl(DeviceImage);
-    applyLinkOptionsFromImage(LinkOptionsStr,
-                              *(InputImpl->get_bin_image_ref()));
+    appendLinkOptionsFromImage(LinkOptionsStr,
+                               *(InputImpl->get_bin_image_ref()));
   }
   const context &Context = getSyclObjImpl(DeviceImages[0])->get_context();
   const ContextImplPtr ContextImpl = getSyclObjImpl(Context);

sycl/unittests/program_manager/passing_link_and_compile_options.cpp

@@ -231,64 +231,9 @@ TEST(Link_Compile_Options, compile_link_Options_Test_filled_options) {
   EXPECT_EQ(expected_compile_options_1, current_compile_options);
 }
 
-TEST(Link_Compile_Options,
-     compile_link_Options_Test_two_devices_filled_options) {
-  sycl::platform Plt{sycl::default_selector()};
-  if (Plt.is_host()) {
-    std::cerr << "Test is not supported on host, skipping\n";
-    GTEST_SKIP(); // test is not supported on host.
-  }
-
-  if (Plt.get_backend() == sycl::backend::ext_oneapi_cuda) {
-    std::cerr << "Test is not supported on CUDA platform, skipping\n";
-    GTEST_SKIP();
-  }
-
-  if (Plt.get_backend() == sycl::backend::ext_oneapi_hip) {
-    std::cerr << "Test is not supported on HIP platform, skipping\n";
-    GTEST_SKIP();
-  }
-  sycl::unittest::PiMock Mock{Plt};
-  setupDefaultMockAPIs(Mock);
-  Mock.redefine<sycl::detail::PiApiKind::piProgramCompile>(
-      redefinedProgramCompile);
-  Mock.redefine<sycl::detail::PiApiKind::piProgramLink>(redefinedProgramLink);
-  const sycl::device Dev_1 = Plt.get_devices()[0];
-  current_link_options.clear();
-  current_compile_options.clear();
-  std::string expected_compile_options_1 = "-cl-opt-disable",
-              expected_compile_options_2 =
-                  "-cl-fp32-correctly-rounded-divide-sqrt",
-              expected_link_options_1 = "-cl-denorms-are-zero",
-              expected_link_options_2 = "-cl-no-signed-zeros";
-  static sycl::unittest::PiImage DevImage_1 =
-      generateEAMTestKernel1Image<EAMTestKernel1>(expected_compile_options_1,
-                                                  expected_link_options_1);
-  static sycl::unittest::PiImage DevImage_2 =
-      generateEAMTestKernel2Image<EAMTestKernel2>(expected_compile_options_2,
-                                                  expected_link_options_2);
-  static sycl::unittest::PiImage Images[] = {DevImage_1, DevImage_2};
-  static sycl::unittest::PiImageArray<2> DevImageArray{Images};
-
-  auto KernelID_1 = sycl::get_kernel_id<EAMTestKernel1>();
-  auto KernelID_2 = sycl::get_kernel_id<EAMTestKernel2>();
-  sycl::queue Queue{Dev_1};
-  const sycl::context Ctx = Queue.get_context();
-  sycl::kernel_bundle KernelBundle1 =
-      sycl::get_kernel_bundle<sycl::bundle_state::input>(Ctx, {Dev_1},
-                                                         {KernelID_1});
-  sycl::kernel_bundle KernelBundle2 =
-      sycl::get_kernel_bundle<sycl::bundle_state::input>(Ctx, {Dev_1},
-                                                         {KernelID_2});
-  auto BundleObj1 = sycl::compile(KernelBundle1);
-  auto BundleObj2 = sycl::compile(KernelBundle2);
-  sycl::link(BundleObj1);
-  sycl::link(BundleObj2);
-  EXPECT_EQ(expected_link_options_1 + " " + expected_link_options_2,
-            current_link_options);
-  EXPECT_EQ(expected_compile_options_1 + " " + expected_compile_options_2,
-            current_compile_options);
-}
+// According to kernel_bundle_impl.hpp:205 sycl::link now is not linking
+// any two device images together
+// TODO : Add check for linking 2 device images together when implemented.
 
 TEST(Link_Compile_Options, check_sycl_build) {
   sycl::platform Plt{sycl::default_selector()};