Address review comments. Disable tests properly.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Author: kbobrovs

sycl/include/CL/sycl/detail/device_binary_image.hpp

@@ -0,0 +1,64 @@
+//==----- device_binary_image.hpp --- SYCL device binary image abstraction -==//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+#pragma once
+
+#include <CL/sycl/detail/os_util.hpp>
+#include <CL/sycl/detail/pi.hpp>
+
+#include <memory>
+
+__SYCL_INLINE_NAMESPACE(cl) {
+namespace sycl {
+namespace detail {
+
+// SYCL RT wrapper over PI binary image.
+class RTDeviceBinaryImage : public pi::DeviceBinaryImage {
+public:
+  RTDeviceBinaryImage(OSModuleHandle ModuleHandle)
+      : pi::DeviceBinaryImage(), ModuleHandle(ModuleHandle) {}
+  RTDeviceBinaryImage(pi_device_binary Bin, OSModuleHandle ModuleHandle)
+      : pi::DeviceBinaryImage(Bin), ModuleHandle(ModuleHandle) {}
+  OSModuleHandle getOSModuleHandle() const { return ModuleHandle; }
+
+  ~RTDeviceBinaryImage() override {}
+
+  bool supportsSpecConstants() const {
+    return getFormat() == PI_DEVICE_BINARY_TYPE_SPIRV;
+  }
+
+  const pi_device_binary_struct &getRawData() const { return *get(); }
+
+  void print() const override {
+    pi::DeviceBinaryImage::print();
+    std::cerr << "    OSModuleHandle=" << ModuleHandle << "\n";
+  }
+
+protected:
+  OSModuleHandle ModuleHandle;
+};
+
+// Dynamically allocated device binary image, which de-allocates its binary
+// data in destructor.
+class DynRTDeviceBinaryImage : public RTDeviceBinaryImage {
+public:
+  DynRTDeviceBinaryImage(std::unique_ptr<char[]> &&DataPtr, size_t DataSize,
+                         OSModuleHandle M);
+  ~DynRTDeviceBinaryImage() override;
+
+  void print() const override {
+    RTDeviceBinaryImage::print();
+    std::cerr << "    DYNAMICALLY CREATED\n";
+  }
+
+protected:
+  std::unique_ptr<char[]> Data;
+};
+
+} // namespace detail
+} // namespace sycl
+} // __SYCL_INLINE_NAMESPACE(cl)

sycl/include/CL/sycl/detail/spec_constant_impl.hpp

@@ -22,7 +22,7 @@ namespace detail {
 // Represents a specialization constant value in SYCL runtime.
 class spec_constant_impl {
 public:
-  spec_constant_impl() = default;
+  spec_constant_impl() : Size(0), Bytes{0} {};
 
   spec_constant_impl(size_t Size, const void *Val) { set(Size, Val); }
 
@@ -33,7 +33,7 @@ class spec_constant_impl {
   bool isSet() const { return Size != 0; }
 
 private:
-  size_t Size; // size of its value
+  size_t Size; // the size of the spec constant value
   // TODO invent more flexible approach to support values of arbitrary type:
   unsigned char Bytes[8]; // memory to hold the value bytes
 };

sycl/include/CL/sycl/experimental/spec_constant.hpp

@@ -24,7 +24,9 @@ __SYCL_INLINE_NAMESPACE(cl) {
 namespace sycl {
 namespace experimental {
 
-class spec_const_error : public compile_program_error {};
+class spec_const_error : public compile_program_error {
+  using compile_program_error::compile_program_error;
+};
 
 template <typename T, typename ID = T> class spec_constant {
 private:

sycl/source/CMakeLists.txt

@@ -106,6 +106,7 @@ set(SYCL_SOURCES
     "detail/common.cpp"
     "detail/config.cpp"
     "detail/context_impl.cpp"
+    "detail/device_binary_image.cpp"
     "detail/device_impl.cpp"
     "detail/error_handling/enqueue_kernel.cpp"
     "detail/event_impl.cpp"

sycl/source/detail/device_binary_image.cpp

@@ -0,0 +1,40 @@
+//==----- device_binary_image.cpp --- SYCL device binary image abstraction -==//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include <CL/sycl/detail/pi.hpp>
+
+#include <memory>
+
+#include <CL/sycl/detail/device_binary_image.hpp>
+
+using namespace sycl::detail;
+
+DynRTDeviceBinaryImage::DynRTDeviceBinaryImage(
+    std::unique_ptr<char[]> &&DataPtr, size_t DataSize, OSModuleHandle M)
+    : RTDeviceBinaryImage(M) {
+  Data = std::move(DataPtr);
+  Bin = new pi_device_binary_struct();
+  Bin->Version = PI_DEVICE_BINARY_VERSION;
+  Bin->Kind = PI_DEVICE_BINARY_OFFLOAD_KIND_SYCL;
+  Bin->DeviceTargetSpec = PI_DEVICE_BINARY_TARGET_UNKNOWN;
+  Bin->CompileOptions = "";
+  Bin->LinkOptions = "";
+  Bin->ManifestStart = nullptr;
+  Bin->ManifestEnd = nullptr;
+  Bin->BinaryStart = reinterpret_cast<unsigned char *>(Data.get());
+  Bin->BinaryEnd = Bin->BinaryStart + DataSize;
+  Bin->EntriesBegin = nullptr;
+  Bin->EntriesEnd = nullptr;
+  Bin->Format = pi::getBinaryImageFormat(Bin->BinaryStart, DataSize);
+  init(Bin);
+}
+
+DynRTDeviceBinaryImage::~DynRTDeviceBinaryImage() {
+  delete Bin;
+  Bin = nullptr;
+}

sycl/source/detail/program_impl.cpp

@@ -485,7 +485,7 @@ void program_impl::flush_spec_constants(const RTDeviceBinaryImage &Img,
 
   auto LockGuard = Ctx->getKernelProgramCache().acquireCachedPrograms();
 
-  for (SCItTy SCIt = SCRange.begin(); SCIt != SCRange.end(); SCIt++) {
+  for (SCItTy SCIt : SCRange) {
     const char *SCName = (*SCIt)->Name;
     auto SCEntry = SpecConstRegistry.find(SCName);
     if (SCEntry == SpecConstRegistry.end())

sycl/source/detail/program_impl.hpp

@@ -315,6 +315,9 @@ class program_impl {
     detail::stableSerializeSpecConstRegistry(SpecConstRegistry, Dst);
   }
 
+  /// Tells whether a specialization constant has been set for this program.
+  bool hasSetSpecConstants() const { return !SpecConstRegistry.empty(); }
+
 private:
   // Deligating Constructor used in Implementation.
   program_impl(ContextImplPtr Context, pi_native_handle InteropProgram,

sycl/source/detail/program_manager/program_manager.cpp

@@ -15,6 +15,7 @@
 #include <CL/sycl/detail/util.hpp>
 #include <CL/sycl/device.hpp>
 #include <CL/sycl/exception.hpp>
+#include <CL/sycl/experimental/spec_constant.hpp>
 #include <CL/sycl/stl.hpp>
 #include <detail/context_impl.hpp>
 #include <detail/device_impl.hpp>
@@ -928,38 +929,15 @@ void ProgramManager::dumpImage(const RTDeviceBinaryImage &Img,
   F.close();
 }
 
-DynRTDeviceBinaryImage::DynRTDeviceBinaryImage(
-    std::unique_ptr<char[]> &&DataPtr, size_t DataSize, OSModuleHandle M)
-    : RTDeviceBinaryImage(M) {
-  Data = std::move(DataPtr);
-  Bin = new pi_device_binary_struct();
-  Bin->Version = PI_DEVICE_BINARY_VERSION;
-  Bin->Kind = PI_DEVICE_BINARY_OFFLOAD_KIND_SYCL;
-  Bin->DeviceTargetSpec = PI_DEVICE_BINARY_TARGET_UNKNOWN;
-  Bin->CompileOptions = "";
-  Bin->LinkOptions = "";
-  Bin->ManifestStart = nullptr;
-  Bin->ManifestEnd = nullptr;
-  Bin->BinaryStart = reinterpret_cast<unsigned char *>(Data.get());
-  Bin->BinaryEnd = Bin->BinaryStart + DataSize;
-  Bin->EntriesBegin = nullptr;
-  Bin->EntriesEnd = nullptr;
-  Bin->Format = pi::getBinaryImageFormat(Bin->BinaryStart, DataSize);
-  init(Bin);
-}
-
-DynRTDeviceBinaryImage::~DynRTDeviceBinaryImage() {
-  delete Bin;
-  Bin = nullptr;
-}
-
 void ProgramManager::flushSpecConstants(const program_impl &Prg,
                                         RT::PiProgram NativePrg,
                                         const RTDeviceBinaryImage *Img) {
   if (DbgProgMgr > 2) {
     std::cerr << ">>> ProgramManager::flushSpecConstants(" << Prg.get()
               << ",...)\n";
   }
+  if (!Prg.hasSetSpecConstants())
+    return; // nothing to do
   pi::PiProgram PrgHandle = Prg.getHandleRef();
   // program_impl can't correspond to two different native programs
   assert(!NativePrg || !PrgHandle || (NativePrg == PrgHandle));
@@ -971,20 +949,20 @@ void ProgramManager::flushSpecConstants(const program_impl &Prg,
       ContextImplPtr Ctx = getSyclObjImpl(Prg.get_context());
       auto LockGuard = Ctx->getKernelProgramCache().acquireCachedPrograms();
       auto It = NativePrograms.find(NativePrg);
-      if (It == NativePrograms.end()) {
-        if (DbgProgMgr > 0)
-          std::cerr << ">>> WARNING: flushSpecConstants requested on a "
-                       "program w/o known binary image\n";
-        return; // program origin is unknown
-      }
+      if (It == NativePrograms.end())
+        throw sycl::experimental::spec_const_error(
+            "spec constant is set in a program w/o a binary image",
+            PI_INVALID_OPERATION);
       Img = It->second;
     }
     if (!Img->supportsSpecConstants()) {
       if (DbgProgMgr > 0)
         std::cerr << ">>> ProgramManager::flushSpecConstants: binary image "
                   << &Img->getRawData() << " doesn't support spec constants\n";
-      // this device binary image does not support runtime setting of
-      // specialization constants; compiler must have generated default values
+      // This device binary image does not support runtime setting of
+      // specialization constants; compiler must have generated default values.
+      // NOTE: Can't throw here, as it would always take place with AOT
+      //-compiled code. New Khronos 2020 spec should fix this inconsistency.
       return;
     }
   }

sycl/source/detail/program_manager/program_manager.hpp

@@ -9,6 +9,7 @@
 #pragma once
 
 #include <CL/sycl/detail/common.hpp>
+#include <CL/sycl/detail/device_binary_image.hpp>
 #include <CL/sycl/detail/export.hpp>
 #include <CL/sycl/detail/os_util.hpp>
 #include <CL/sycl/detail/pi.hpp>
@@ -51,49 +52,6 @@ enum DeviceLibExt {
   cl_intel_devicelib_complex_fp64
 };
 
-// SYCL RT wrapper over PI binary image.
-class RTDeviceBinaryImage : public pi::DeviceBinaryImage {
-public:
-  RTDeviceBinaryImage(OSModuleHandle ModuleHandle)
-      : pi::DeviceBinaryImage(), ModuleHandle(ModuleHandle) {}
-  RTDeviceBinaryImage(pi_device_binary Bin, OSModuleHandle ModuleHandle)
-      : pi::DeviceBinaryImage(Bin), ModuleHandle(ModuleHandle) {}
-  OSModuleHandle getOSModuleHandle() const { return ModuleHandle; }
-
-  ~RTDeviceBinaryImage() override {}
-
-  bool supportsSpecConstants() const {
-    return getFormat() == PI_DEVICE_BINARY_TYPE_SPIRV;
-  }
-
-  const pi_device_binary_struct &getRawData() const { return *get(); }
-
-  void print() const override {
-    pi::DeviceBinaryImage::print();
-    std::cerr << "    OSModuleHandle=" << ModuleHandle << "\n";
-  }
-
-protected:
-  OSModuleHandle ModuleHandle;
-};
-
-// Dynamically allocated device binary image, which de-allocates its binary data
-// in destructor.
-class DynRTDeviceBinaryImage : public RTDeviceBinaryImage {
-public:
-  DynRTDeviceBinaryImage(std::unique_ptr<char[]> &&DataPtr, size_t DataSize,
-                         OSModuleHandle M);
-  ~DynRTDeviceBinaryImage() override;
-
-  void print() const override {
-    RTDeviceBinaryImage::print();
-    std::cerr << "    DYNAMICALLY CREATED\n";
-  }
-
-protected:
-  std::unique_ptr<char[]> Data;
-};
-
 // Provides single loading and building OpenCL programs with unique contexts
 // that is necessary for no interoperability cases with lambda.
 class ProgramManager {

sycl/test/spec_const/spec_const_hw.cpp

@@ -1,9 +1,11 @@
 // RUN: %clangxx -fsycl %s -o %t.out
 // RUN: env SYCL_DEVICE_TYPE=HOST %t.out
-// TODO: re-enable after OpenCL RT is fixed:
-// RUNx: %CPU_RUN_PLACEHOLDER %t.out
-// RUNx: %GPU_RUN_PLACEHOLDER %t.out
-// RUNx: %ACC_RUN_PLACEHOLDER %t.out
+// RUN: %CPU_RUN_PLACEHOLDER %t.out
+// RUN: %GPU_RUN_PLACEHOLDER %t.out
+// RUN: %ACC_RUN_PLACEHOLDER %t.out
+// TODO: re-enable after CI drivers are updated to newer which support spec
+// constants:
+// XFAIL: acc,cpu,cuda,gen
 //
 //==----------- spec_const_hw.cpp ------------------------------------------==//
 //

sycl/test/spec_const/spec_const_redefine.cpp

@@ -1,11 +1,11 @@
 // RUN: %clangxx -fsycl %s -o %t.out
 // RUN: env SYCL_DEVICE_TYPE=HOST %t.out
-// TODO re-enable CPU device test once CI CPU OCL RT is updated:
-// RUNx: env SYCL_PI_TRACE=2 %CPU_RUN_PLACEHOLDER %t.out 2>&1 %CPU_CHECK_PLACEHOLDER
+// RUN: env SYCL_PI_TRACE=2 %CPU_RUN_PLACEHOLDER %t.out 2>&1 %CPU_CHECK_PLACEHOLDER
 // RUN: env SYCL_PI_TRACE=2 %GPU_RUN_PLACEHOLDER %t.out 2>&1 %GPU_CHECK_PLACEHOLDER
 // RUN: env SYCL_PI_TRACE=2 %ACC_RUN_PLACEHOLDER %t.out 2>&1 %ACC_CHECK_PLACEHOLDER
-// TODO the test currently fails on these devices:
-// XFAIL: acc,cuda
+// TODO: re-enable after CI drivers are updated to newer which support spec
+// constants:
+// XFAIL: acc,cpu,cuda,gen
 //
 //==----------- spec_const_redefine.cpp ------------------------------------==//
 //