[SYCL] Enable PI unit testing on multiple plugins. #1694

nyalloc · 2020-05-15T15:33:58Z

This PR re-enables all disabled PI API unit tests.

Backend-agnostic unit tests have been refactored using value-parameterized tests, allowing the same set of unit tests to be ran on a set of PI plugins.

Signed-off-by: Stuart Adams [email protected]

Enable CUDA + OpenCL PI unit tests. Signed-off-by: Stuart Adams <[email protected]>

Signed-off-by: Stuart Adams <[email protected]>

bader · 2020-05-15T17:04:25Z

Okay. The failure seems to be sporadic.
I see SYCL-Unit::CudaInteropGetNativeTests.interopTaskGetMem failed twice for the PRs, which can't impact test results on CUDA.

http://ci.llvm.intel.com:8010/#/builders/37/builds/869 from [SYCL] Add dependency.conf with external dependencies for SYCL #1555.
http://ci.llvm.intel.com:8010/#/builders/37/builds/859 from [SYCL] Add support for kernel name types templated using enums. #1675.

OTOH, I see a lot of successful runs.

bader · 2020-05-15T17:17:50Z

@StuartDAdams, are you able to reproduce this failure in your environment?

pvchupin · 2020-05-15T18:36:54Z

We should disable sporadic fails until it's resolved.

nyalloc · 2020-05-18T17:37:39Z

@StuartDAdams, are you able to reproduce this failure in your environment?

Nothing is failing for me yet. This is curious.

… pi_platforms

Signed-off-by: Stuart Adams <[email protected]>

nyalloc · 2020-05-19T15:59:26Z

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

@@ -1422,9 +1429,9 @@ struct _pi_plugin {
  // Some choices are:
  // - Use of integers to keep major and minor version.
  // - Keeping char* Versions.
-  const char PiVersion[4] = _PI_H_VERSION_STRING;


This const member was causing issues, as the compiler was no longer able to generate a default copy assignment operator, making it difficult to pass a vector of plugins to the algorithms I use in this PR. I could write one by hand, but copy constructors and other C++ language features have no place in this C API header.

Additionally, these in-class member initialisers are a C++ language feature. If I'm not mistaken, it generates a default constructor where these two members are initalized. Because of this, I moved the initialisation of these members to pi::initialize function in pi.cpp. _pi_plugin is now a POD because of these changes. There are a few C++ features in pi.h that would fail to compile with C, but these are the only things relevant to this PR.

nyalloc · 2020-05-19T16:10:36Z

sycl/unittests/pi/TestGetPlugin.hpp

+  return *it;
+}
+
+inline std::vector<cl::sycl::detail::plugin> initializeAndRemoveInvalid() {


@bader @smaslov-intel

I took on your feeback from #1647, and came up with this. In the version of googletest that ships with LLVM, there is no way to skip tests at runtime (googletest would later introduce GTEST_SKIP).

Instead I have written a wrapper around pi::initiaize that removes plugins that will not behave well during unit testing. If the plugin cannot return any platforms or it returns an error code, it is dropped from the test. Using this wrapper, the parameterised tests will now run tests only for plugins that are able to return platforms. I plan on adding a log for this as well, so that any dropped plugins can be seen in test logs.

sycl/unittests/pi/TestGetPlugin.hpp

bader · 2020-05-20T11:19:24Z

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

+  PI_DEVICE_TYPE_ALL =
+      CL_DEVICE_TYPE_ALL, ///< All devices available in the PI plugin.
+  PI_DEVICE_TYPE_CPU =
+      CL_DEVICE_TYPE_CPU, ///< A PI device that is the host processor.


I'm not sure it's a 100% accurate. I assume it's possible to have CPU device != host processor.

This is the wording OpenCL uses for their equivalent enumerator. I guess what you're thinking of would be more along the lines of an accelerator?

This is what I used as reference.

https://www.khronos.org/registry/OpenCL/sdk/1.0/docs/man/xhtml/clGetDeviceIDs.html

Interesting... I always thought that OpenCL spec doesn't force the implementation to use the host processor on multi-processor systems. I think it's possible to configure Intel OpenCL CPU implementation to run on a "non-host" processor.

This might be just an typo in the spec...
@mkinsner, @bashbaug, do you have any thoughts regarding this wording?

In my view, PI API is very close to OpenCL by design so it makes sense to me to keep the language here similar to the original wording. If it turns out to be a typo in the OpenCL spec, I'll happily update it with a follow-up PR. It is important that we re-enable PI API testing ASAP.

It's very minor comment and it shouldn't block merging this PR.

This same text is in the latest OpenCL spec also. It hasn't changed since v1.0:

https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_API.html#CL_DEVICE_TYPE_CPU

I'll get this tidied up in the spec. I agree it shouldn't hold up this PR.

bader · 2020-05-20T17:48:55Z

@intel/llvm-reviewers-runtime, ping.

alexbatashev

Comments are stylistic, they should not block this PR.

alexbatashev · 2020-05-20T18:48:20Z

sycl/unittests/pi/BackendString.hpp

+    return "Unknown Plugin";
+  }
+}
+} // namespace pi


It is screaming for a blank line.

alexbatashev · 2020-05-20T18:49:34Z

sycl/unittests/pi/BackendString.hpp

@@ -0,0 +1,23 @@
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.


The header seems to be a little different from what the rest of the project uses.

alexbatashev · 2020-05-20T18:57:19Z

sycl/source/detail/plugin.hpp

+  plugin &operator=(const plugin &) = default;
+  plugin(const plugin &) = default;
+  plugin &operator=(plugin &&other) noexcept = default;
+  plugin(plugin &&other) noexcept = default;


Interesting... What made you explicitly default these things?

The compiler was deleting a few of these implicitly because pi_plugin's constant members. I was trying to narrow in on the source of the error, as it prevented me from passing plugins to standard algorithms that required the type to be copy assignable. Because this type encapsulates pi_plugin object, the compiler decided to delete its copy assignment and copy constructor. Now that pi_plugin has been fixed, you could probably remove these explicit defaults to no ill effect, but I decided to leave them in. I prefer all special member functions to be explicitly defined if any of them are present, as I can reason about the class' behavior without having to guess at what the compiler will do.

bader · 2020-05-20T19:11:49Z

Let's address non-blocking comments in follow-up PR.

nyalloc added 7 commits May 6, 2020 14:32

Backend agnostic PI tests are parameterised by PI Plugin.

95977a6

Enable CUDA + OpenCL PI unit tests. Signed-off-by: Stuart Adams <[email protected]>

Clang format

512741d

Signed-off-by: Stuart Adams <[email protected]>

Print PI API backend name when running tests

c448c80

Signed-off-by: Stuart Adams <[email protected]>

Documented pi_device_type

39a85a8

Signed-off-by: Stuart Adams <[email protected]>

Implemented PR feedback, cleaned up new header files.

a07f0f7

Removed using namespace from unit test header.

0f9e310

Clang format

8cc36a6

Signed-off-by: Stuart Adams <[email protected]>

nyalloc requested review from smaslov-intel and a team as code owners May 15, 2020 15:33

nyalloc requested a review from s-kanaev May 15, 2020 15:33

nyalloc changed the title ~~[WIP][SYCL] Enable PI unit testing on multiple plugins.~~ [WIP][DO-NOT-MERGE][SYCL] Enable PI unit testing on multiple plugins. May 15, 2020

nyalloc added 3 commits May 19, 2020 16:42

initializeAndRemoveInvalid function gets all plugins with more than 0…

f18759d

… pi_platforms

clang format

6cfc9d9

Clang format

174315e

Signed-off-by: Stuart Adams <[email protected]>

nyalloc commented May 19, 2020

View reviewed changes

smaslov-intel reviewed May 19, 2020

View reviewed changes

sycl/unittests/pi/TestGetPlugin.hpp Outdated Show resolved Hide resolved

Update TestGetPlugin implementation based on PR feedback.

944b660

nyalloc requested a review from smaslov-intel May 20, 2020 10:53

Print error message when a plugin is removed from testing

e15d752

nyalloc changed the title ~~[WIP][DO-NOT-MERGE][SYCL] Enable PI unit testing on multiple plugins.~~ [SYCL] Enable PI unit testing on multiple plugins. May 20, 2020

bader approved these changes May 20, 2020

View reviewed changes

smaslov-intel approved these changes May 20, 2020

View reviewed changes

v-klochkov approved these changes May 20, 2020

View reviewed changes

alexbatashev approved these changes May 20, 2020

View reviewed changes

bader merged commit a6381fe into intel:sycl May 20, 2020

		@@ -0,0 +1,23 @@
		// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.

[SYCL] Enable PI unit testing on multiple plugins. #1694

[SYCL] Enable PI unit testing on multiple plugins. #1694

Uh oh!

Conversation

nyalloc commented May 15, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

pvchupin commented May 15, 2020

Uh oh!

nyalloc commented May 18, 2020

Uh oh!

nyalloc May 19, 2020 • 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!

nyalloc May 19, 2020 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyalloc May 20, 2020 • 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!

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!

bader commented May 20, 2020

Uh oh!

alexbatashev 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!

bader commented May 20, 2020

Uh oh!

Uh oh!

nyalloc May 19, 2020 •

edited

Loading

nyalloc May 19, 2020 •

edited

Loading

nyalloc May 20, 2020 •

edited

Loading