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

nyalloc · 2020-05-06T13:37:17Z

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]>

vladimirlaz

Approve to start tests

vladimirlaz · 2020-05-06T17:00:56Z

@StuartDAdams, could you please:

Add [SYCL] tag to commit message
Point me to the place where all available plugins are selected for a test and how it is going to be presented in test logs?
Fix clang-format problem

sycl/unittests/pi/EnqueueMemTest.cpp

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

nyalloc · 2020-05-07T12:42:30Z

@StuartDAdams, could you please:

Add [SYCL] tag to commit message

Point me to the place where all available plugins are selected for a test and how it is going to be presented in test logs?

Fix clang-format problem

Right now the tests operate on all plugins returned by pi::initialize().

https://github.com/intel/llvm/pull/1647/files#diff-c2020da2ebb84661f358e463c350b60aR103
https://github.com/intel/llvm/pull/1647/files#diff-b2b6029a4a9ce6ad7a167388a276e67dR76
https://github.com/intel/llvm/pull/1647/files#diff-3c1288974e170a703a99c7f76a2cd4baR64

This vector is then passed to the test fixture through INSTANTIATE_TEST_CASE_P.

Regarding logging, I could create a new function that returns a string literal for the plugin type. When an assertion fails, it will print the appropriate message and the plugin type that it failed on. Would this be sufficient?

vladimirlaz · 2020-05-08T07:26:10Z

will print the appropriate message and the plugin
@StuartDAdams Thank you for clarification.
It would be great to have information about faulty plugin in the error logs.

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

nyalloc · 2020-05-08T13:21:43Z

will print the appropriate message and the plugin
@StuartDAdams Thank you for clarification.
It would be great to have information about faulty plugin in the error logs.

I've updated the PR so that the name of the PI plugin is now printed for each test run. The output looks like this:

[----------] 12 tests from EventTestImpl/EventTest
[ RUN      ] EventTestImpl/EventTest.PICreateEvent/opencl
[       OK ] EventTestImpl/EventTest.PICreateEvent/opencl (81 ms)
[ RUN      ] EventTestImpl/EventTest.PICreateEvent/cuda
[       OK ] EventTestImpl/EventTest.PICreateEvent/cuda (58 ms)
[ RUN      ] EventTestImpl/EventTest.piEventSetCallback/opencl
        Event callback 0 of type 2 triggered
        Event callback 1 of type 1 triggered
        Event callback 2 of type 0 triggered
[       OK ] EventTestImpl/EventTest.piEventSetCallback/opencl (69 ms)
[ RUN      ] EventTestImpl/EventTest.piEventSetCallback/cuda
        Event callback 0 of type 2 triggered
        Event callback 1 of type 1 triggered
        Event callback 2 of type 0 triggered
[       OK ] EventTestImpl/EventTest.piEventSetCallback/cuda (31 ms)
[ RUN      ] EventTestImpl/EventTest.piEventGetInfo/opencl
[       OK ] EventTestImpl/EventTest.piEventGetInfo/opencl (0 ms)
[ RUN      ] EventTestImpl/EventTest.piEventGetInfo/cuda
[       OK ] EventTestImpl/EventTest.piEventGetInfo/cuda (52 ms)
[ RUN      ] EventTestImpl/EventTest.piEventSetStatus/opencl
[       OK ] EventTestImpl/EventTest.piEventSetStatus/opencl (0 ms)
[ RUN      ] EventTestImpl/EventTest.piEventSetStatus/cuda
[       OK ] EventTestImpl/EventTest.piEventSetStatus/cuda (50 ms)
[ RUN      ] EventTestImpl/EventTest.WaitForManualEventOnOtherThread/opencl
[       OK ] EventTestImpl/EventTest.WaitForManualEventOnOtherThread/opencl (0 ms)
[ RUN      ] EventTestImpl/EventTest.WaitForManualEventOnOtherThread/cuda
[       OK ] EventTestImpl/EventTest.WaitForManualEventOnOtherThread/cuda (51 ms)
[ RUN      ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl
[       OK ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl (0 ms)
[ RUN      ] EventTestImpl/EventTest.piEnqueueEventsWait/cuda
[       OK ] EventTestImpl/EventTest.piEnqueueEventsWait/cuda (28 ms)
[----------] 12 tests from EventTestImpl/EventTest (420 ms total)

vladimirlaz · 2020-05-11T05:53:51Z

[ RUN      ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl
[       OK ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl (0 ms)

Some tests take 0 ms on opencl backend (50+ms on cuda) it it expected or test is skipped. What is the source or overhead on cuda?

nyalloc · 2020-05-11T12:11:43Z

[ RUN      ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl
[       OK ] EventTestImpl/EventTest.piEnqueueEventsWait/opencl (0 ms)
Some tests take 0 ms on opencl backend (50+ms on cuda) it it expected or test is skipped. What is the source or overhead on cuda?

This output is from a debug build, so there could be a number of things increasing the overhead of the tests. In the interest of keeping this PR focused, I think it should only enable the disabled PI API unit tests, and leave any potential performance issues for future issues and PRs.

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

bader

A couple of style comments.

sycl/unittests/pi/cuda/test_get_plugin.hpp

sycl/unittests/pi/BackendString.hpp

sycl/unittests/pi/cuda/test_get_plugin.hpp

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

bader

Thanks!

bader · 2020-05-14T10:45:02Z

@StuartDAdams, 7 tests are failing on the systems w/o OpenCL.

From: https://github.com/intel/llvm/runs/673664063

Failing Tests (7):
SYCL-Unit :: pi/./PiTests/EnqueueMemTestImpl/EnqueueMemTest.piEnqueueMemBufferFill/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.PICreateEvent/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.WaitForManualEventOnOtherThread/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEnqueueEventsWait/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventGetInfo/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventSetCallback/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventSetStatus/opencl

Testing Time: 810.52s
Unsupported Tests : 45
Expected Passes : 224
Expected Failures : 1
Unexpected Failures: 7
5 warning(s) in tests

bader · 2020-05-15T09:00:19Z

@StuartDAdams, more serious problem seems to be exposed here: #1675 (comment). I think it's more serious because it impacts pre-commit testing and we need to fix this ASAP.
Could you take a look, please?

nyalloc · 2020-05-15T13:13:42Z

@StuartDAdams, 7 tests are failing on the systems w/o OpenCL.

From: https://github.com/intel/llvm/runs/673664063

Failing Tests (7):
SYCL-Unit :: pi/./PiTests/EnqueueMemTestImpl/EnqueueMemTest.piEnqueueMemBufferFill/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.PICreateEvent/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.WaitForManualEventOnOtherThread/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEnqueueEventsWait/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventGetInfo/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventSetCallback/opencl
SYCL-Unit :: pi/./PiTests/EventTestImpl/EventTest.piEventSetStatus/opencl

Testing Time: 810.52s
Unsupported Tests : 45
Expected Passes : 224
Expected Failures : 1
Unexpected Failures: 7
5 warning(s) in tests

Why is the OpenCL plugin returned by pi::initialize() when the systems do not have OpenCL? These tests simply run for all plugins returned by that pi::initialize(). If that function returns invalid plugins, then it needs to be fixed. This should probably be opened as a seperate issue.

bader · 2020-05-15T13:30:05Z

Looks like a bug to me.
initialize relies on findPlugins, which always returns OpenCL and CUDA plug-ins, w/o trying to "find" them.
https://github.com/intel/llvm/blob/sycl/sycl/source/detail/pi.cpp#L257
https://github.com/intel/llvm/blob/sycl/sycl/source/detail/pi.cpp#L208

Tagging @romanovvlad and @smaslov-intel.

bader · 2020-05-15T13:36:29Z

Looking more closely at the code of initialize method, I don't really understand what is the purpose of this function.
If it's supposed to just "load SYCL plug-in", than it probably does it right.
The problem is that although OpenCL plug-in is loaded, it can't be used because there are OpenCL implementations suitable for running OpenCL plug-in.

nyalloc · 2020-05-15T13:49:11Z

My understanding was this: if it has not been called previously, it looks at what backends are available and inititalises them, then returns their plugin tables so that you may choose which you would like to use. If it has been called previously, it returns the plugins that it previously initialized.

While I have not looked at the code yet, it sounds like the function is hard-coded to return an OpenCL plugin even if one is not available. If that is the case, I think the function should be updated so that it only returns the available plugins.

nyalloc · 2020-05-15T14:48:27Z

Discussion is continuted here: #1693

…1693) This reverts commit 98119bd.

bader · 2020-05-15T15:18:31Z

While I have not looked at the code yet, it sounds like the function is hard-coded to return an OpenCL plugin even if one is not available. If that is the case, I think the function should be updated so that it only returns the available plugins.

The plug-in is "available" i.e. it's loaded and it might be that even OpenCL ICD loader is available. The problem is initialize doesn't check that OpenCL API is implemented or not.
I'm not sure is it the right place for this check or not.
@romanovvlad, @smaslov-intel, could you help to clarify this?

smaslov-intel · 2020-05-15T16:40:49Z

The pi::initialize finds (currently hard-coded library names), loads (does dlopen of those libraries), and binds (call piPluginInit entry in those libraries). Only those plugins that successfully passed all the steps above are returned, and I think this is the correct behavior.

If the issue is that OpenCL lacks any platforms it can operate, then it would just return 0 platforms in piPlatformsGet later. Why is this causing any issues? We may consider enhancing the "bind" step to check for non-0 platforms available. Currently it just does this: https://github.com/intel/llvm/blob/sycl/sycl/plugins/opencl/pi_opencl.cpp#L1076

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

We need to define the behavior for all plug-ins and I think we should keep existing behavior. Otherwise we might open Pandora's box with questions like "okay we check a platform, but what about device?", etc.
I suggest adjust the test expectation. E.g. replace this assert with an early exit: https://github.com/StuartDAdams/llvm/blob/8cc36a6883764065063ee5da7338bfde01f27bb0/sycl/unittests/pi/EnqueueMemTest.cpp#L37

nyalloc · 2020-05-19T10:30:37Z

We need to define the behavior for all plug-ins and I think we should keep existing behavior. Otherwise we might open Pandora's box with questions like "okay we check a platform, but what about device?", etc.
I suggest adjust the test expectation. E.g. replace this assert with an early exit: https://github.com/StuartDAdams/llvm/blob/8cc36a6883764065063ee5da7338bfde01f27bb0/sycl/unittests/pi/EnqueueMemTest.cpp#L37

I think this is an acceptable workaround for now. I will update the open PR.

Backend agnostic PI tests are parameterised by PI Plugin.

95977a6

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

nyalloc requested review from smaslov-intel and a team as code owners May 6, 2020 13:37

nyalloc requested a review from vladimirlaz May 6, 2020 13:37

vladimirlaz previously approved these changes May 6, 2020

View reviewed changes

vladimirlaz self-requested a review May 6, 2020 16:50

smaslov-intel reviewed May 7, 2020

View reviewed changes

sycl/unittests/pi/EnqueueMemTest.cpp Show resolved Hide resolved

nyalloc changed the title ~~Enable PI unit testing on multiple plugins.~~ [SYCL] Enable PI unit testing on multiple plugins. May 7, 2020

Clang format

512741d

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

nyalloc dismissed vladimirlaz’s stale review via 512741d May 7, 2020 12:19

nyalloc requested a review from a team as a code owner May 7, 2020 12:19

nyalloc requested a review from smaslov-intel May 7, 2020 12:54

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]>

nyalloc force-pushed the stuart/enable_unit_tests branch from 6a600b7 to 39a85a8 Compare May 11, 2020 15:56

smaslov-intel previously approved these changes May 11, 2020

View reviewed changes

vladimirlaz previously approved these changes May 12, 2020

View reviewed changes

bader reviewed May 12, 2020

View reviewed changes

sycl/unittests/pi/cuda/test_get_plugin.hpp Show resolved Hide resolved

sycl/unittests/pi/BackendString.hpp Show resolved Hide resolved

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

sycl/unittests/pi/cuda/test_get_plugin.hpp Show resolved Hide resolved

Implemented PR feedback, cleaned up new header files.

a07f0f7

nyalloc dismissed stale reviews from vladimirlaz and smaslov-intel via a07f0f7 May 12, 2020 12:30

bader reviewed May 12, 2020

View reviewed changes

sycl/unittests/pi/cuda/test_get_plugin.hpp Outdated Show resolved Hide resolved

nyalloc added 2 commits May 12, 2020 14:11

Removed using namespace from unit test header.

0f9e310

Clang format

8cc36a6

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

bader approved these changes May 12, 2020

View reviewed changes

nyalloc requested review from vladimirlaz and smaslov-intel May 12, 2020 15:23

smaslov-intel approved these changes May 12, 2020

View reviewed changes

vladimirlaz approved these changes May 14, 2020

View reviewed changes

bader merged commit 98119bd into intel:sycl May 14, 2020

bader pushed a commit that referenced this pull request May 15, 2020

Revert "[SYCL] Enable PI unit testing on multiple plugins. (#1647)" (#…

5ad2b58

…1693) This reverts commit 98119bd.

bader mentioned this pull request May 15, 2020

Revert "[SYCL] Enable PI unit testing on multiple plugins. (#1647)" #1693

Merged

nyalloc mentioned this pull request May 19, 2020

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

Merged

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

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

Uh oh!

Conversation

nyalloc commented May 6, 2020

Uh oh!

vladimirlaz left a comment

Choose a reason for hiding this comment

Uh oh!

vladimirlaz commented May 6, 2020

Uh oh!

Uh oh!

nyalloc commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladimirlaz commented May 8, 2020

Uh oh!

nyalloc commented May 8, 2020

Uh oh!

vladimirlaz commented May 11, 2020

Uh oh!

nyalloc commented May 11, 2020

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented May 14, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

nyalloc commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented May 15, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

nyalloc commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyalloc commented May 15, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

smaslov-intel commented May 15, 2020

Uh oh!

bader commented May 15, 2020

Uh oh!

nyalloc commented May 19, 2020

Uh oh!

Uh oh!

nyalloc commented May 7, 2020 •

edited

Loading

nyalloc commented May 15, 2020 •

edited

Loading

nyalloc commented May 15, 2020 •

edited

Loading