[SYCL] Addition of suggested changes from PR#808 and PR#843.

Signed-off-by: Garima Gupta <[email protected]>

Author: garimagu

sycl/include/CL/sycl/detail/pi.h

@@ -917,13 +917,18 @@ pi_result piEnqueueMemUnmap(
 #define STRING_HELPER(a) #a
 #define STRINGIZE(a,b) STRING_HELPER(a.b)
 
-struct pi_plugin {
+struct _pi_plugin{
   // PI version supported by host passed to the plugin. The Plugin
   // checks and writes the appropriate Function Pointers in
-  // PIFunctionTable.
+  // PiFunctionTable.
+  // TODO: Work on version fields and their handshaking mechanism.
+  // Some choices are: 
+  // - Use of integers to keep major and minor version.
+  // - Keeping char* Versions.
   const char PiVersion[4] = STRINGIZE(_PI_H_VERSION_MAJOR,_PI_H_VERSION_MINOR);
+  // Plugin edits this.
   char PluginVersion[4] =
-      STRINGIZE(_PI_H_VERSION_MAJOR,_PI_H_VERSION_MINOR); // Plugin edits this.
+      STRINGIZE(_PI_H_VERSION_MAJOR,_PI_H_VERSION_MINOR);
   char *Targets;
   struct FunctionPointers {
 #define _PI_API(api) decltype(::api) *api;

sycl/include/CL/sycl/detail/pi.hpp

@@ -119,16 +119,9 @@ void printArgs(Arg0 arg0, Args... args) {
 
 // Utility function to check return from pi calls.
 // Throws if pi_result is not a PI_SUCCESS.
-// TODO: Absorb this utility in Trace Class
-template <typename Exception> inline void piCheckThrow(PiResult pi_result) {
-  CHECK_OCL_CODE_THROW(pi_result, Exception);
-}
-
-// Utility function to check if return from pi call is
-// PI_SUCCESS. If is it not, throw a cl::sycl::runtime_error.
-// TODO: Absorb this utility in Trace Class
+template <typename Exception = cl::sycl::runtime_error>
 inline void piCheckResult(PiResult pi_result) {
-  piCheckThrow<cl::sycl::runtime_error>(pi_result);
+  CHECK_OCL_CODE_THROW(pi_result, Exception);
 }
 
 #define PI_TRACE_ENABLED (std::getenv("SYCL_PI_TRACE") != nullptr)
@@ -141,30 +134,32 @@ template <typename FnType, size_t FnOffset> class Trace {
 public:
   Trace();
   template <typename... Args> PiResult operator()(Args... args) {
-    if (PI_TRACE_ENABLED) {
+    bool enableTrace = PI_TRACE_ENABLED;
+    if (enableTrace) {
       std::cout << "---> " << m_FnName << "(";
       printArgs(args...);
     }
 
     PiResult r = m_FnPtr(args...);
 
-    if (PI_TRACE_ENABLED) {
+    if (enableTrace) {
       std::cout << ") ---> ";
       std::cout << (print(r), "") << std::endl;
     }
     return r;
   }
 };
 
-template <typename FnType, size_t FnOffset>
+template <typename FnType, size_t FnOffset,
+          typename Exception = cl::sycl::runtime_error>
 class TraceCheck : private Trace<FnType, FnOffset> {
 public:
   TraceCheck() : Trace<FnType, FnOffset>(){};
 
-  template <typename Exception = cl::sycl::runtime_error, typename... Args>
+  template <typename... Args>
   void operator()(Args... args) {
     PiResult Err = (Trace<FnType, FnOffset>::operator()(args...));
-    piCheckThrow<Exception>(Err);
+    piCheckResult<Exception>(Err);
   }
 };
 
@@ -174,7 +169,7 @@ class TraceCheck : private Trace<FnType, FnOffset> {
 #define _PI_API(api)                                                           \
   template <>                                                                  \
   Trace<decltype(&::api),                                                      \
-        (offsetof(_pi_plugin::FunctionPointers, api))>::Trace();
+        (offsetof(pi_plugin::FunctionPointers, api))>::Trace();
 
 #include <CL/sycl/detail/pi.def>
 
@@ -187,23 +182,25 @@ namespace RT = cl::sycl::detail::pi;
 // Use this macro to call the API, trace the call, check the return and throw a
 // runtime_error exception.
 // Usage: PI_CALL(pi)(Args);
-// Note: To change the exception type, use:
-// PI_CALL(pi).template operator()<compile_program_error>(__VA_ARGS__)
-// Or
-// auto Err = PI_CALL_NOCHECK(pi)(args);
-// RT::piCheckThrow<Exception>(Err);
 #define PI_CALL(pi)                                                            \
   RT::TraceCheck<decltype(&::pi),                                              \
-                 (offsetof(_pi_plugin::FunctionPointers, pi))>()
+                 (offsetof(pi_plugin::FunctionPointers, pi))>()
 
 // Use this macro to call the API, trace the call and return the result.
-// To check the result use piCheckResult or piCheckThrow.
+// To check the result use piCheckResult.
 // Usage:
 // PiResult Err = PI_CALL_NOCHECK(pi)(args);
 // RT::piCheckResult(Err); <- Checks Result and throws a runtime error
 // exception.
 #define PI_CALL_NOCHECK(pi)                                                    \
-  RT::Trace<decltype(&::pi), (offsetof(_pi_plugin::FunctionPointers, pi))>()
+  RT::Trace<decltype(&::pi), (offsetof(pi_plugin::FunctionPointers, pi))>()
+
+// Use this macro to call the API, trace the call, check the return and throw a
+// Exception as given in the MACRO.
+// Usage: PI_CALL_THROW(pi, compile_program_error)(args);
+#define PI_CALL_THROW(pi, Exception)                                           \
+  RT::TraceCheck<decltype(&::pi),                                              \
+                 (offsetof(pi_plugin::FunctionPointers, pi)), Exception>()
 
 // Want all the needed casts be explicit, do not define conversion
 // operators.

sycl/include/CL/sycl/detail/program_impl.hpp

@@ -89,12 +89,10 @@ class program_impl {
         NonInterOpToLink |= !Prg->IsLinkable;
         Programs.push_back(Prg->Program);
       }
-      RT::PiResult Err = PI_SUCCESS;
-      Err = PI_CALL_NOCHECK(piProgramLink)(
+      PI_CALL_THROW(piProgramLink, compile_program_error)(
           detail::getSyclObjImpl(Context)->getHandleRef(), Devices.size(),
           Devices.data(), LinkOptions.c_str(), Programs.size(), Programs.data(),
           nullptr, nullptr, &Program);
-      RT::piCheckThrow<compile_program_error>(Err);
     }
   }
 
@@ -247,11 +245,10 @@ class program_impl {
       check_device_feature_support<
           info::device::is_linker_available>(Devices);
       vector_class<RT::PiDevice> Devices(get_pi_devices());
-      RT::PiResult Err = PI_CALL_RESULT(piProgramLink)(
+      PI_CALL_THROW(piProgramLink, compile_program_error)(
           detail::getSyclObjImpl(Context)->getHandleRef(), Devices.size(),
           Devices.data(), LinkOptions.c_str(), 1, &Program, nullptr, nullptr,
           &Program);
-      RT::piCheckThrow<compile_program_error>(Err);
       this->LinkOptions = LinkOptions;
       BuildOptions = LinkOptions;
     }

sycl/include/CL/sycl/detail/sycl_mem_obj_t.hpp

@@ -86,13 +86,13 @@ template <typename AllocatorT> class SYCLMemObjT : public SYCLMemObjI {
 
     RT::PiMem Mem = pi::cast<RT::PiMem>(MInteropMemObject);
     RT::PiContext Context = nullptr;
-    RT::piCheckResult(PI_CALL_NOCHECK(piMemGetInfo)(
-        Mem, CL_MEM_CONTEXT, sizeof(Context), &Context, nullptr));
+    PI_CALL(piMemGetInfo)(
+        Mem, CL_MEM_CONTEXT, sizeof(Context), &Context, nullptr);
 
     if (MInteropContext->getHandleRef() != Context)
       throw cl::sycl::invalid_parameter_error(
           "Input context must be the same as the context of cl_mem");
-    RT::piCheckResult(PI_CALL_NOCHECK(piMemRetain)(Mem));
+    PI_CALL(piMemRetain)(Mem);
   }
 
   SYCLMemObjT(cl_mem MemObject, const context &SyclContext,

sycl/plugins/opencl/pi_opencl.cpp

@@ -460,14 +460,14 @@ pi_result OCL(piEnqueueMemBufferMap)(
 }
 
 pi_result piPluginInit(pi_plugin *PluginInit) {
-  strcpy(PluginInit->PluginVersion, SupportedVersion);
   int CompareVersions = strcmp(PluginInit->PiVersion, SupportedVersion);
   if (CompareVersions < 0) {
     // PI interface supports lower version of PI.
-    assert(false && "incompatible versions.!!\n");
+    // TODO: Take appropriate actions.
     return PI_INVALID_OPERATION;
   } else {
     // PI interface supports higher version or the same version.
+    strncpy(PluginInit->PluginVersion, SupportedVersion, 4);
 
 #define _PI_CL(pi_api, ocl_api)                                                \
   (PluginInit->PiFunctionTable).pi_api = (decltype(&::pi_api))(&ocl_api);

sycl/source/detail/pi.cpp

@@ -80,17 +80,12 @@ bool bindPlugin(void *Library) {
   decltype(::piPluginInit) *PluginInitializeFunction = (decltype(
       &::piPluginInit))(getOsLibraryFuncAddress(Library, "piPluginInit"));
   int err = PluginInitializeFunction(&PluginInformation);
-  int CompareVersions =
-      strcmp(PluginInformation.PiVersion, PluginInformation.PluginVersion);
-
-  // CompareVersions >= 0, Plugin Interface supports same/higher PI version as
-  // the Plugin.
-  // TODO: When Plugin supports lower version of PI, check for backward
-  // compatibility.
-  assert((CompareVersions >= 0) && "Plugin Interface supports lower PI version "
-                                   "than Plugin. Update library.");
-  // Reaching here means CompareVersions>=0, make sure err is PI_SUCCESS.
+
+  // TODO: Compare Supported versions and check for backward compatibility.
+  // Make sure err is PI_SUCCESS.
   assert((err == PI_SUCCESS) && "Unexpected error when binding to Plugin.");
+  
+  // TODO: Return a more meaningful value/enum.
   return true;
 }
 
@@ -150,7 +145,7 @@ void assertion(bool Condition, const char *Message) {
 #define _PI_API(api)                                                           \
   template <>                                                                  \
   Trace<decltype(&::api),                                                      \
-        (offsetof(_pi_plugin::FunctionPointers, api))>::Trace() {              \
+        (offsetof(pi_plugin::FunctionPointers, api))>::Trace() {              \
     initialize();                                                              \
     m_FnPtr = (RT::PluginInformation.PiFunctionTable.api);                     \
     m_FnName = #api;                                                           \

sycl/unittests/pi/PlatformTest.cpp

@@ -32,8 +32,7 @@ class PlatformTest : public ::testing::Test {
     // Initialize the logged number of platforms before the following assertion.
     RecordProperty(platform_count_key, platform_count);
 
-    ASSERT_EQ(RT::PluginInformation.PiFunctionTable.piPlatformsGet(
-                  0, 0, &platform_count),
+    ASSERT_EQ(PI_CALL_NOCHECK(piPlatformsGet)(0, 0, &platform_count),
               PI_SUCCESS);
 
     // Overwrite previous log value with queried number of platforms.
@@ -50,8 +49,8 @@ class PlatformTest : public ::testing::Test {
 
     _platforms.resize(platform_count, nullptr);
 
-    ASSERT_EQ(RT::PluginInformation.PiFunctionTable.piPlatformsGet(
-                  _platforms.size(), _platforms.data(), nullptr),
+    ASSERT_EQ(PI_CALL_NOCHECK(piPlatformsGet)(_platforms.size(),
+                                              _platforms.data(), nullptr),
               PI_SUCCESS);
   }
 };
@@ -64,13 +63,13 @@ TEST_F(PlatformTest, piPlatformsGet) {
 TEST_F(PlatformTest, piPlatformGetInfo) {
   auto get_info_test = [](pi_platform platform, _pi_platform_info info) {
     size_t reported_string_length = 0;
-    EXPECT_EQ(RT::PluginInformation.PiFunctionTable.piPlatformGetInfo(
-                  platform, info, 0u, nullptr, &reported_string_length),
+    EXPECT_EQ(PI_CALL_NOCHECK(piPlatformGetInfo)(platform, info, 0u, nullptr,
+                                                 &reported_string_length),
               PI_SUCCESS);
 
     // Create a larger result string to catch overwrites.
     std::vector<char> param_value(reported_string_length * 2u, '\0');
-    EXPECT_EQ(RT::PluginInformation.PiFunctionTable.piPlatformGetInfo(
+    EXPECT_EQ(PI_CALL_NOCHECK(piPlatformGetInfo)(
                   platform, info, param_value.size(), param_value.data(), 0u),
               PI_SUCCESS)
         << "piPlatformGetInfo for " << RT::platformInfoToString(info)