Skip to content

Feature/backend specific queues v2 #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

diptorupd
Copy link
Contributor

Rebased on top of master.

Diptorup Deb and others added 8 commits October 3, 2020 11:07
   -- Queues now need to be queried using both backend and device type.
   -- Fix test cases.
   -- Platform interfaces now can return the list of available backends.
   -- Fix skiped tests.
   -- Expand the platform interface to provide backend info.
   -- Move backend enums to platfrom from context.
   -- Add comments.
   -- Store opencl_cpu, opencl_gpu, and level0_gpu queues inside queue manager.
   -- Fixed enum typedefs, added cython dict to store backends and device types.
About static variables.
@diptorupd diptorupd requested a review from PokhodenkoSA October 3, 2020 21:24
@diptorupd diptorupd marked this pull request as draft October 3, 2020 21:25
     - Rename SyclQueueManager to SyclRTManager
     - Minor stylistic changes:
          -- reorder functions alphabetically.
	  -- Move all enums into separate file.
     - Add C API to get backend from queue,
       and also add a way to check if a queue is the current queue.
     - Update sycl_core to match C API.
@diptorupd diptorupd force-pushed the feature/backend_specific_queues_v2 branch from 33decb1 to c96c50d Compare October 3, 2020 23:49
Diptorup Deb added 4 commits October 3, 2020 20:20
    -- Added a way to check if two DPPLSyclQueueRef and the SyclQueue
       wrappers point to the same sycl queue.
    -- Same thing for DPPLSyclContextRef and SyclContext.
@diptorupd diptorupd marked this pull request as ready for review October 4, 2020 05:57
@diptorupd
Copy link
Contributor Author

diptorupd commented Oct 4, 2020

Ready for review.

This PR is the last major thing that I wanted to get into Beta 10. Among other things it makes sure we always know what type of back end we are selecting.

Fixes several important issues:
#53
#73
#51
#33
#95
#83

@diptorupd diptorupd force-pushed the feature/backend_specific_queues_v2 branch from 87f3460 to d6bcd74 Compare October 4, 2020 14:23
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small questions and remarks from me. Not worth to block the PR. LGTM.

@oleksandr-pavlyk
Copy link
Contributor

Requires gtest 1.10 to build (I was not able to execute build_for_develop with gtest 1.8)

@oleksandr-pavlyk
Copy link
Contributor

Error handling needs to be improved:

In [1]: import dpctl

In [2]: with dpctl.device_context("cpu::0"):
   ...:     print(1)
   ...:
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
/localdisk/work/opavlyk/repos/dpctl/dpctl/sycl_core.pyx in dpctl._sycl_core._SyclQueueManager._set_as_current_queue()

KeyError: 'cpu'

During handling of the above exception, another exception occurred:

UnsupportedBackendError                   Traceback (most recent call last)
<ipython-input-2-de76f0cd9073> in <module>
----> 1 with dpctl.device_context("cpu::0"):
      2     print(1)
      3

~/.conda/envs/idp/lib/python3.7/contextlib.py in __enter__(self)
    110         del self.args, self.kwds, self.func
    111         try:
--> 112             return next(self.gen)
    113         except StopIteration:
    114             raise RuntimeError("generator didn't yield") from None

/localdisk/work/repos/dpctl/dpctl/sycl_core.pyx in device_context()

/localdisk/work/repos/dpctl/dpctl/sycl_core.pyx in dpctl._sycl_core._SyclQueueManager._set_as_current_queue()

UnsupportedBackendError: Backend can only be opencl or level-0

Note that I am not sure what the arguments of device_context changed too, so I was trying something I though might be sensible.

@oleksandr-pavlyk
Copy link
Contributor

Python 3.7.7 (default, Jul 14 2020, 22:02:37)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.17.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import dpctl

In [2]: with dpctl.device_context('level0:gpu:0'):
   ...:     print(dpctl.get_current_queue().get_sycl_device().dump_device_info())
   ...:
TBB Warning: The number of workers is currently limited to 11. The request for 31 workers is ignored. Further requests for more workers will be silently ignored until the limit changes.

    Name            Intel(R) Gen9
    Driver version  1.0.17767
    Vendor          Intel(R) Corporation
    Profile         FULL_PROFILE
    Device type     gpu
None

In [3]: with dpctl.device_context('level0:gpu:0'):
   ...:     dpctl.get_current_queue().get_sycl_device().dump_device_info()
   ...:
    Name            Intel(R) Gen9
    Driver version  1.0.17767
    Vendor          Intel(R) Corporation
    Profile         FULL_PROFILE
    Device type     gpu

In [4]: with dpctl.device_context('opencl:gpu:0'):
   ...:     dpctl.get_current_queue().get_sycl_device().dump_device_info()
   ...:
    Name            Intel(R) Gen9 HD Graphics NEO
    Driver version  20.35.17767
    Vendor          Intel(R) Corporation
    Profile         FULL_PROFILE
    Device type     gpu

In [5]:

Notice the TBB warning on the first invocation.

It would be nice if dpctl provided a function to enumerate all possible arguments understood by device_context, even if only to be used to construct an informative error message.

@oleksandr-pavlyk
Copy link
Contributor

I think this is good overall, and should be merged asap.

Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall.

@oleksandr-pavlyk
Copy link
Contributor

The TBB message is coming from auto ctx = context(d) line in QMgrHelper::init_queues when d is a GPU device.

In [1]: import dpctl

In [2]: with dpctl.device_context('opencl:gpu'):
   ...:     print(1)
   ...:
at the start
before get info
after get info
after get context
before get info
after get info
TBB Warning: The number of workers is currently limited to 11. The request for 31 workers is ignored. Further requests for more workers will be silently ignored until the limit changes.

after get context
before get info
after get info
after get context
before emplace
after emplace
after new
1

@oleksandr-pavlyk
Copy link
Contributor

Please do not merge. I root caused the TBB messages, and preparing a fix

Solution is not to create context on devices that are not
of the requested device type or not from the requested backend.

```
Python 3.7.7 (default, Jul 14 2020, 22:02:37)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.17.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import dpctl

In [2]: with dpctl.device_context('opencl:gpu'):
   ...:     print(1)
      ...:
      1

In [3]: with dpctl.device_context('level0:gpu'):
   ...:     print(1)
      ...:
      1

```
@oleksandr-pavlyk
Copy link
Contributor

oleksandr-pavlyk commented Oct 5, 2020

Ok, I pushed the fix. Please look it over and merge.

* CC=clang.exe

* CC=clang-cl.exe
@diptorupd
Copy link
Contributor Author

Merging now. Thanks for the reviews!

@diptorupd diptorupd merged commit 251f135 into IntelPython:master Oct 5, 2020
@diptorupd diptorupd deleted the feature/backend_specific_queues_v2 branch October 5, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants