Change PI_CALL syntax from PI_CALL(pi, ...Args) to PI_CALL(pi)(...Args); #843

garimagu · 2019-11-19T00:44:41Z

Changes done:

Keeping PI API function pointers in a structure. Passing this structure to the plugin. The plugin populates the function pointers appropriately. This makes us have a single call to the plugin to bind all APIs. 2 commits.
Commit 3 changes the PI_CALL syntax. The call looks like a constructor call and then a functor call on the constructor.
Commit 4,5 does other formatting changes and introduces the PI_CALL_THROW Macro.

garimagu · 2019-11-19T00:57:46Z

review only the last commit.

garimagu · 2019-11-19T00:58:29Z

Some cleaning of comments is needed.

romanovvlad · 2019-11-19T15:27:42Z

It would be nice if you put rationale for such changes in the commit message.

AlexeySachkov · 2019-11-20T12:22:17Z

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;
-    PI_CALL(RT::piMemGetInfo, Mem, CL_MEM_CONTEXT, sizeof(Context), &Context,
-            nullptr);
+    RT::piCheckResult(PI_CALL_NOCHECK(piMemGetInfo)(


This looks...kind of strange: what so specific is being done by piCheckResult that cannot be done by regular PI_CALL?

I don't think we even need piCheckResult. We should write error specific result checking for each pi invocation.

This was a mistake. I think it may have happened because of the changes being done in stages.In some intermediate stage I have chosen PI_CALL_RESULT for some reason and then changed it to this directly.

I don't think we even need piCheckResult. We should write error specific result checking for each pi invocation.

Are you saying at each PI_CALL site, we should check the error return code and perform an action appropriately?
The usage of piCheckResult() API or in PI_CALL, means that the site of call only wants to check if a PI_SUCCESS is received and it throws a runtime_error.

@romanovvlad Please confirm if i understood you correct/addressed your concern.

AlexeySachkov · 2019-11-20T12:27:49Z

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

+    if (PI_TRACE_ENABLED) {
+      std::cout << "---> " << m_FnName << "(";
      printArgs(args...);
+    }

-    auto r = m_FnPtr(args...);
+    PiResult r = m_FnPtr(args...);

-    if (m_TraceEnabled) {
+    if (PI_TRACE_ENABLED) {
      std::cout << ") ---> ";
      std::cout << (print(r), "") << std::endl;
    }
    return r;


So, +2 getenv calls on each PI API call, right? I guess this is some kind of overkill and definitely will affect performance

changed the code.

bjoernknafla · 2019-11-20T20:23:28Z

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

+#define _PI_API(api)                                                           \
+  template <>                                                                  \
+  Trace<decltype(&::api),                                                      \
+        (offsetof(_pi_plugin::FunctionPointers, api))>::Trace();


Is the implementation missing here (might be further below)?

Is this specialization required? Might it work to declare the constructor inside the Trace class as Trace() = default; (and declaring the function pointer inside the class as follows FnType m_FnPtr = nullptr; ?

The implementation is in pi.cpp
I'm sorry I didn't understand your suggestion.

@bjoernknafla Can you explain your suggestion again?

garimagu

added suggested changes to commit#5

garimagu · 2019-11-20T19:17:08Z

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;
-    PI_CALL(RT::piMemGetInfo, Mem, CL_MEM_CONTEXT, sizeof(Context), &Context,
-            nullptr);
+    RT::piCheckResult(PI_CALL_NOCHECK(piMemGetInfo)(


This was a mistake. I think it may have happened because of the changes being done in stages.In some intermediate stage I have chosen PI_CALL_RESULT for some reason and then changed it to this directly.

garimagu · 2019-11-20T19:20:26Z

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;
-    PI_CALL(RT::piMemGetInfo, Mem, CL_MEM_CONTEXT, sizeof(Context), &Context,
-            nullptr);
+    RT::piCheckResult(PI_CALL_NOCHECK(piMemGetInfo)(


I don't think we even need piCheckResult. We should write error specific result checking for each pi invocation.

Are you saying at each PI_CALL site, we should check the error return code and perform an action appropriately?
The usage of piCheckResult() API or in PI_CALL, means that the site of call only wants to check if a PI_SUCCESS is received and it throws a runtime_error.

garimagu · 2019-11-20T19:21:33Z

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

+    if (PI_TRACE_ENABLED) {
+      std::cout << "---> " << m_FnName << "(";
      printArgs(args...);
+    }

-    auto r = m_FnPtr(args...);
+    PiResult r = m_FnPtr(args...);

-    if (m_TraceEnabled) {
+    if (PI_TRACE_ENABLED) {
      std::cout << ") ---> ";
      std::cout << (print(r), "") << std::endl;
    }
    return r;


changed the code.

garimagu · 2019-11-20T20:40:15Z

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

+#define _PI_API(api)                                                           \
+  template <>                                                                  \
+  Trace<decltype(&::api),                                                      \
+        (offsetof(_pi_plugin::FunctionPointers, api))>::Trace();


The implementation is in pi.cpp
I'm sorry I didn't understand your suggestion.

garimagu · 2019-11-20T22:16:57Z

@smaslov-intel Please check the changes.
I am yet to make the clang-format updates.

garimagu · 2019-11-21T02:43:52Z

@smaslov-intel Please check the clang-format changes.
@erichkeane @bader : Is it ok to add changes in the clang-format config file in llvm directory?

erichkeane · 2019-11-21T03:10:19Z

Is it possible to just alter the clang-format at a much lower directory? It seems no one alters the 'root' version ever in community. BUT, I suspect you could add sometihng at the /sycl/include level.

garimagu · 2019-11-21T17:56:54Z

Is it possible to just alter the clang-format at a much lower directory? It seems no one alters the 'root' version ever in community. BUT, I suspect you could add sometihng at the /sycl/include level.

Yes. I can do that. There is no .clang-format file in sycl folder.
Can i add it there? Or should I make two, one for each include and source directory?

erichkeane · 2019-11-21T17:57:56Z

Is it possible to just alter the clang-format at a much lower directory? It seems no one alters the 'root' version ever in community. BUT, I suspect you could add sometihng at the /sycl/include level.

Yes. I can do that. There is no .clang-format file in sycl folder.
Can i add it there? Or should I make two, one for each include and source directory?

If it'll work, I think just the root 'sycl' directory would be fine.

smaslov-intel

Mostly looks good to me, just rather minor suggestions.

smaslov-intel · 2019-11-21T23:04:13Z

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

-// TODO: Absorb this utility in Trace Class
-template <typename Exception> inline void piCheckThrow(PiResult pi_result) {
+template <typename Exception = cl::sycl::runtime_error>
+inline void piCheckResult(PiResult pi_result) {


we decided to not prepend with "pi" utilities in "pi" namespace, so this would be just checkResult, or maybe better check PiResult

smaslov-intel · 2019-11-21T23:05:39Z

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

  template <typename... Args> PiResult operator()(Args... args) {
-    if (m_TraceEnabled)
+    bool enableTrace = PI_TRACE_ENABLED;


It is still a getenv() per each PI call, which is redundant. Please make this a static member of the class and initialize just once.

smaslov-intel · 2019-11-21T23:10:03Z

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


-template <typename FnType> class Trace {
+template <typename FnType, size_t FnOffset> class Trace {


Please add comments about what this class is doing, and I think it should be named "Call", and not "Trace" as this is its primary functionality. Also a good place to explain in comment what the template arguments are for.

Changed to CallPi.

smaslov-intel · 2019-11-21T23:13:14Z

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

-bool Trace<FnType>::m_TraceEnabled = (std::getenv("SYCL_PI_TRACE") != nullptr);
+template <typename FnType, size_t FnOffset,
+          typename Exception = cl::sycl::runtime_error>
+class TraceCheck : private Trace<FnType, FnOffset> {


TraceCheck -> CallAndCheck

changed to CallPiAndCheck

smaslov-intel · 2019-11-21T23:14:09Z

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

      std::cout << ") ---> ";
      std::cout << (print(r), "") << std::endl;
    }
    return r;
  }
 };

-template <typename FnType>
-bool Trace<FnType>::m_TraceEnabled = (std::getenv("SYCL_PI_TRACE") != nullptr);
+template <typename FnType, size_t FnOffset,


This is the header, please explain the interface here, i.e. what the exception argument stands for, and what the class is for

done. Please see if it looks good.

smaslov-intel · 2019-11-21T23:16:42Z

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

-    RT::initialize();                                                          \
-    RT::piCheckResult(PI_TRACE(pi)(__VA_ARGS__));                              \
-  }
+#define PI_ASSERT(cond, msg) RT::assertion((cond), "assert: " msg);


why do we need this macro?

in Pi::Cast. Moved it closer to the API.

smaslov-intel · 2019-11-21T23:23:28Z

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


-#define PI_TRACE_ONLY(func) RT::Trace<decltype(func)>(func, #func)
+namespace RT = cl::sycl::detail::pi;


I suggest to rename RT to PI (not in this change-set please)

Sure. One change-set just for formatting and Naming.

And one for the version and version handshaking.

smaslov-intel · 2019-11-21T23:28:30Z

sycl/plugins/opencl/pi_opencl.cpp

-const char SupportedVersion[] = "1.1";
+#define STRING_HELPER(a) #a
+#define STRINGIZE(a,b) STRING_HELPER(a.b)
+const char SupportedVersion[] = STRINGIZE(_PI_H_VERSION_MAJOR,_PI_H_VERSION_MINOR);


since the arguments are defined in pi.h why not define the string there too? And then you don't need to repeat stringizing utilities here

I wanted to define a PI_H_VERSION_STRING macro, which uses _PI_H_VERSION_MAJOR, _PI_H_VERSION_MINOR.
but then I didn't want to define the STRINGIZE macros in the .cpp file.
I have made some changes. Let me know what you think.

garimagu · 2019-11-22T22:16:44Z

Rebase and conflict resolution.

garimagu · 2019-11-22T22:17:45Z

@smaslov-intel Please check the changes done.

smaslov-intel

LGTM

smaslov-intel · 2019-11-22T22:32:15Z

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

+//    FnOffset- Offset to the Function Pointer in the piPlugin::FunctionPointers
+//    structure.Used to differentiate between APIs with same pointer type, Eg:
+//    piDeviceRelease and piDeviceRetain. Differentiation needed to avoid
+//    duplicate instantiation of class in pi.cpp.


please clarify that this is really a stability issue, i.e. you cannot do different template specializations for different PI interfaces (having same type) without this

smaslov-intel · 2019-11-23T07:51:12Z

sycl/source/detail/pi.cpp

-  template <>                                                                  \
-  CallPi<decltype(&::api),                                                      \
-        (offsetof(pi_plugin::FunctionPointers, api))>::CallPi() {               \
+  template <> CallPi<decltype(&::api)>::CallPi() {                             \


where had the offset parameter gone?

Sorry about that. I changed the code to check the exact error message I get; changed it back. tested it, but during final cleaning of the code included the wrong PiCall Definition.

datastructure with all function pointers. Extension to this will allow us to bind multiple plugins. Signed-off-by: Garima Gupta <[email protected]>

PI_CALL( or PI_CALL_RESULT( and other required changes. Signed-off-by: Garima Gupta <[email protected]>

to PI_CALL(pi)(...Args); Required extension of Trace class. Changed the syntax at all call sites. Signed-off-by: Garima Gupta <[email protected]>

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

Changed the clang-format config file to consider PI_CALL/PI_CALL_THROW/PI_CALL_NOCHECK as typenames instead of being functions. Signed-off-by: Garima Gupta <[email protected]>

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

introduced during rebase-conflict resolution. Bug fix in unittests/pi/PlatformTest.cpp. Signed-off-by: Garima Gupta <[email protected]>

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

Rectified merge conflict issues. Signed-off-by: Garima Gupta <[email protected]>

garimagu · 2019-11-24T00:18:58Z

rebased, resolved merge conflict issues.

smaslov-intel

Thanks! Looks good to me.

bader

LGTM. A few minor comments.

bader · 2019-11-25T09:50:44Z

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

+// Template Arguments:
+//    FnType  - Type of Function pointer to the PI API.
+//    FnOffset- Offset to the Function Pointer in the piPlugin::FunctionPointers
+//    structure. Used to differentiate between APIs with same pointer type, Eg:


Suggested change

// structure. Used to differentiate between APIs with same pointer type, Eg:

// structure. Used to differentiate between APIs with same pointer type, E.g.:

bader · 2019-11-25T09:51:03Z

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

+//    structure. Used to differentiate between APIs with same pointer type, Eg:
+//    piDeviceRelease and piDeviceRetain. Differentiation needed to avoid
+//    redefinition error during explicit specialization of class in pi.cpp.
+// Members: Initialiaed in default constructor in Class Template Specialization.


Suggested change

// Members: Initialiaed in default constructor in Class Template Specialization.

// Members: Initialized in default constructor in Class Template Specialization.

bader · 2019-11-25T09:53:13Z

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

+// Template Arguments:
+//    FnType, FnOffset - for CallPi Class.
+//    Exception - The type of exception to throw if PiResult of a call is not
+//    PI_SUCCESS. default is cl::sycl::runtime_error.


Suggested change

// PI_SUCCESS. default is cl::sycl::runtime_error.

// PI_SUCCESS. Default value is cl::sycl::runtime_error.

bader · 2019-11-25T09:56:02Z

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

+// To check the result use checkPiResult.
+// Usage:
+// PiResult Err = PI_CALL_NOCHECK(pi)(args);
+// RT::checkPiResult(Err); <- Checks Result and throws a runtime error


Suggested change

// RT::checkPiResult(Err); <- Checks Result and throws a runtime error

// RT::checkPiResult(Err); <- Checks Result and throws a runtime_error

bader · 2019-11-25T09:59:12Z

sycl/plugins/opencl/pi_opencl.cpp

+    // PI interface supports lower version of PI.
+    // TODO: Take appropriate actions.
+    return PI_INVALID_OPERATION;
+  } else {


Please, don't use else after return.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

I think Bjoern suggested this too. I forgot to do the change.

bader · 2019-11-25T10:01:37Z

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

+#define PI_CALL_NOCHECK(pi)                                                    \
+  RT::CallPi<decltype(&::pi), (offsetof(pi_plugin::FunctionPointers, pi))>()
+
+// Use this macro to call the API, trace the call, check the return and throw a


I think "throw a Exception" should be either "throw an Exception" or "throw the Exception".

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

garimagu · 2019-11-25T10:48:20Z

@bader Please approve and merge.

garimagu assigned smaslov-intel Nov 19, 2019

garimagu mentioned this pull request Nov 19, 2019

[SYCL] Plugin Interface: Single Call to Plugin to populate a Plugin Datastructure. #808

Closed

AlexeySachkov reviewed Nov 20, 2019

View reviewed changes

bjoernknafla reviewed Nov 20, 2019

View reviewed changes

garimagu commented Nov 20, 2019

View reviewed changes

garimagu force-pushed the private/garimagu/pi_callsemanticchange branch from 1000107 to be01470 Compare November 21, 2019 19:19

smaslov-intel suggested changes Nov 21, 2019

View reviewed changes

garimagu force-pushed the private/garimagu/pi_callsemanticchange branch from be01470 to caa1506 Compare November 22, 2019 22:15

smaslov-intel previously approved these changes Nov 22, 2019

View reviewed changes

garimagu dismissed smaslov-intel’s stale review via ac5617f November 23, 2019 07:25

smaslov-intel reviewed Nov 23, 2019

View reviewed changes

garimagu added 8 commits November 23, 2019 16:07

[SYCL] Using a single call to Plugin, and populating a Plugin

709722d

datastructure with all function pointers. Extension to this will allow us to bind multiple plugins. Signed-off-by: Garima Gupta <[email protected]>

[SYCL] Changing all the PI_CALL(RT:: or PI_CALL_RESULT(RT:: to

40d9593

PI_CALL( or PI_CALL_RESULT( and other required changes. Signed-off-by: Garima Gupta <[email protected]>

[SYCL] Change PI_CALL syntax from PI_CALL(pi, ...Args)

505505a

to PI_CALL(pi)(...Args); Required extension of Trace class. Changed the syntax at all call sites. Signed-off-by: Garima Gupta <[email protected]>

Changes - formatting and usage of PI_H_VERISON_** macros.

f60cb6c

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

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

fa380f3

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

Change of PI_CALLS into proper format.

29b516b

Changed the clang-format config file to consider PI_CALL/PI_CALL_THROW/PI_CALL_NOCHECK as typenames instead of being functions. Signed-off-by: Garima Gupta <[email protected]>

Move of .clang-format file to sycl folder.

5a7969d

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

Bug fix in include/CL/sycl/detail/program_impl.hpp, due to error

945dd85

introduced during rebase-conflict resolution. Bug fix in unittests/pi/PlatformTest.cpp. Signed-off-by: Garima Gupta <[email protected]>

garimagu added 3 commits November 23, 2019 16:14

[SYCL] Addition of suggestions.

600096c

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

Clarifications in comments and some formatting changes.

7cc25c4

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

Rectification of a stupid error.

f8480a0

Rectified merge conflict issues. Signed-off-by: Garima Gupta <[email protected]>

garimagu force-pushed the private/garimagu/pi_callsemanticchange branch from ac5617f to f8480a0 Compare November 24, 2019 00:18

smaslov-intel previously approved these changes Nov 24, 2019

View reviewed changes

bader reviewed Nov 25, 2019

View reviewed changes

Suggested changes in comments.

69e96c0

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

garimagu dismissed smaslov-intel’s stale review via 69e96c0 November 25, 2019 10:46

bader approved these changes Nov 25, 2019

View reviewed changes

bader merged commit e3b76be into intel:sycl Nov 25, 2019


		template <typename FnType> class Trace {
		template <typename FnType, size_t FnOffset> class Trace {


		#define PI_TRACE_ONLY(func) RT::Trace<decltype(func)>(func, #func)
		namespace RT = cl::sycl::detail::pi;

	// structure. Used to differentiate between APIs with same pointer type, Eg:
	// structure. Used to differentiate between APIs with same pointer type, E.g.:

	// Members: Initialiaed in default constructor in Class Template Specialization.
	// Members: Initialized in default constructor in Class Template Specialization.

	// PI_SUCCESS. default is cl::sycl::runtime_error.
	// PI_SUCCESS. Default value is cl::sycl::runtime_error.

	// RT::checkPiResult(Err); <- Checks Result and throws a runtime error
	// RT::checkPiResult(Err); <- Checks Result and throws a runtime_error

Change PI_CALL syntax from PI_CALL(pi, ...Args) to PI_CALL(pi)(...Args); #843

Change PI_CALL syntax from PI_CALL(pi, ...Args) to PI_CALL(pi)(...Args); #843

Uh oh!

Conversation

garimagu commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garimagu commented Nov 19, 2019

Uh oh!

garimagu commented Nov 19, 2019

Uh oh!

romanovvlad commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernknafla Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garimagu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garimagu commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garimagu commented Nov 21, 2019

Uh oh!

erichkeane commented Nov 21, 2019

Uh oh!

garimagu commented Nov 21, 2019

Uh oh!

erichkeane commented Nov 21, 2019

Uh oh!

smaslov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garimagu commented Nov 19, 2019 •

edited

Loading

bjoernknafla Nov 20, 2019 •

edited

Loading

garimagu commented Nov 20, 2019 •

edited

Loading

garimagu Nov 24, 2019 •

edited

Loading