Skip to content

[SYCL] Make device id unique per backend #3611

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 56 commits into from
Closed

[SYCL] Make device id unique per backend #3611

wants to merge 56 commits into from

Conversation

bso-intel
Copy link
Contributor

@bso-intel bso-intel commented Apr 23, 2021 •
edited
Loading

We decided to make device id numbers unique per backend.
Also, by adding the device_type into each device prefix listing in sycl-ls,
the user can easily set SYCL_DEVICE_FILTER correctly.

Signed-off-by: Byoungro So [email protected]

@bso-intel
[SYCL] Add device_type into sycl-ls prefix
e898390 
By adding the device_type into each device prefix listing in sycl-ls,
the user can easily set SYCL_DEVICE_FILTER correctly.

Signed-off-by: Byoungro So <[email protected]>
@bso-intel bso-intel requested review from kbobrovs, pvchupin and a team as code owners April 23, 2021 22:33
@bso-intel bso-intel requested a review from againull April 23, 2021 22:33
@bso-intel
Copy link
Contributor Author

[Before]

[opencl:0] ACC : Intel(R) FPGA Emulation Platform for OpenCL(TM) 1.2 [2021.11.3.0.09_160000]
[opencl:0] GPU : Intel(R) OpenCL HD Graphics 3.0 [21.12.19358]
[opencl:0] CPU : Intel(R) OpenCL 2.1 [2021.11.3.0.09_160000]
[level_zero:0] GPU : Intel(R) Level-Zero 1.0 [1.0.19358]
[host:0] HOST: SYCL host platform 1.2 [1.2]

[After]

[opencl:acc:0] : Intel(R) FPGA Emulation Platform for OpenCL(TM) 1.2 [2021.11.3.0.09_160000]
[opencl:gpu:0] : Intel(R) OpenCL HD Graphics 3.0 [21.12.19358]
[opencl:cpu:0] : Intel(R) OpenCL 2.1 [2021.11.3.0.09_160000]
[level_zero:gpu:0] : Intel(R) Level-Zero 1.0 [1.0.19358]
[host:host:0] : SYCL host platform 1.2 [1.2]

@bso-intel bso-intel requested a review from smaslov-intel April 23, 2021 22:37
vladimirlaz
vladimirlaz previously approved these changes Apr 24, 2021
View reviewed changes
smaslov-intel
smaslov-intel suggested changes Apr 26, 2021
View reviewed changes
@bso-intel bso-intel marked this pull request as draft April 26, 2021 22:25
@bso-intel
changed to static_cast
018e5ad 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel bso-intel changed the title [SYCL] Add device_type into sycl-ls prefix [SYCL] Make device id unique per backend Apr 27, 2021
@bso-intel
uppercase device type
2cd0eae 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
revert uppercase
4f6d9c4 
Signed-off-by: Byoungro So <[email protected]>
vladimirlaz
vladimirlaz reviewed Apr 28, 2021
View reviewed changes
@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request Apr 28, 2021
Merged
smaslov-intel
smaslov-intel suggested changes Apr 28, 2021
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

More issues for the new changes

bso-intel and others added 2 commits April 28, 2021 10:32
bso-intel added 5 commits July 17, 2021 18:23
@bso-intel
optimize device_impl
e8be632 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
trial revert of windows fix
e8a26da 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
tokenize filter
d3b9cda 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
try to disable cleaning up device cache
1dc38df 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
revert
d2e967a 
Signed-off-by: Byoungro So <[email protected]>
romanovvlad
romanovvlad reviewed Jul 26, 2021
View reviewed changes
bso-intel and others added 3 commits July 26, 2021 12:12
romanovvlad
romanovvlad previously approved these changes Jul 27, 2021
View reviewed changes
@bso-intel
Copy link
Contributor Author

@smaslov-intel
I addressed your request to refactor the device/platform cache in SYCL RT.

smaslov-intel
smaslov-intel previously approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

Please add test for new indexing (per backend, not per platform).

@bso-intel
Copy link
Contributor Author

Please add test for new indexing (per backend, not per platform).

Here is an example of the current output of sycl-ls.

[opencl:cpu:0] : Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz 2.1 [2021.12.6.0.19_160000]
[opencl:acc:1] : Intel(R) FPGA Emulation Device 1.2 [2021.12.6.0.19_160000]
[opencl:gpu:2] : Intel(R) UHD Graphics 630 [0x3e92] 3.0 [21.19.19792]
[level_zero:gpu:0] : Intel(R) UHD Graphics 630 [0x3e92] 1.1 [1.1.19792]
[host:host:0] : SYCL host device 1.2 [1.2]

It is hard to add a test that checks the device id because a device id changes in different systems.

@smaslov-intel
Copy link
Contributor

Please add test for new indexing (per backend, not per platform).

Here is an example of the current output of sycl-ls.

[opencl:cpu:0] : Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz 2.1 [2021.12.6.0.19_160000]
[opencl:acc:1] : Intel(R) FPGA Emulation Device 1.2 [2021.12.6.0.19_160000]
[opencl:gpu:2] : Intel(R) UHD Graphics 630 [0x3e92] 3.0 [21.19.19792]
[level_zero:gpu:0] : Intel(R) UHD Graphics 630 [0x3e92] 1.1 [1.1.19792]
[host:host:0] : SYCL host device 1.2 [1.2]
It is hard to add a test that checks the device id because a device id changes in different systems.

I suggest you check generally that there are no duplicate entries in the [ID] output of sycl-ls.

@bso-intel
trial tests
19de5f4 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel bso-intel dismissed stale reviews from smaslov-intel and romanovvlad via 19de5f4 July 29, 2021 01:22
@bso-intel
Copy link
Contributor Author

@smaslov-intel ,
I added two tests that checks the duplicate device id per backend.

romanovvlad
romanovvlad reviewed Jul 29, 2021
View reviewed changes
sycl/test/regression/sycl-ls-unique-device-id-level-zero.cpp Outdated

// RUN: env SYCL_DEVICE_FILTER=level_zero sycl-ls | FileCheck %s --check-prefixes=CHECK-OPENCL

// CHECK-OPENCL-COUNT-1: [level_zero:{{.*}}:0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clarify how this check works?
Also, could you please move tests that require low-level runtime to llvm-test-suite?

Copy link
Contributor Author

@bso-intel bso-intel Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This // ***-COUNT-# directive only matches the given pattern only # times. In this case, we try to match only once.
I will move these tests to llvm-test-suite.
I wanted to confirm these tests with my PR changeset.

Copy link
Contributor

@romanovvlad romanovvlad Jul 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, this check verifies that the tool prints a Level Zero device with id 0 if Level Zero is available, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it verifies that the given pattern (level_zero with device id 0) should be exactly matched once.
For example, if sycl-ls prints [level_zero:cpu:0] and [level_zero:gpu:0], this test will fail.

@bso-intel
try Windows shutdown
7f02fc3 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel bso-intel marked this pull request as draft August 2, 2021 18:50
@bso-intel
Copy link
Contributor Author

Please DO NOT MERGE this PR.
I will keep this PR as a draft for a while due to other issues.
I will close this PR once all related issues are fixed.

@bader
Copy link
Contributor

bader commented Aug 19, 2021

Looks like similar patch is uploaded to another pull request - #4247.
@bso-intel, what is the plan for this one?

@bso-intel
Copy link
Contributor Author

Looks like similar patch is uploaded to another pull request - #4247.
@bso-intel, what is the plan for this one?

@bader ,
This PR will be closed eventually.
This PR is divided into 2 parts.
The first one is #4247. The second one is not created yet.
The second part is blocked by other dependency on OpenMP runtime.
So, my plan is to keep this PR until OpenMP team resolves their issue.
Once it is resolved, I will take the rest of this PR and create the second PR.
This PR is currently kept only for my future work. (this became a draft PR)
Thanks.

vladimirlaz pushed a commit to intel/llvm-test-suite that referenced this pull request Sep 24, 2021
@bso-intel
[SYCL] Change pattern to reflect the change in sycl-ls (#256)
0ad1054 
In intel/llvm#3611, I changed the device type to lower case which was necessary to show the SYCL_DEVICE_FILTER prefix for each device listing from sycl-ls.
It caused several tests failures that expected 'GPU' instead of 'gpu'.
@bader
Copy link
Contributor

bader commented Jan 12, 2022

@bso-intel, can we close this PR?

@bso-intel
Copy link
Contributor Author

@bader Yes, I will close this PR.
Thanks.

@bso-intel bso-intel closed this Jan 12, 2022
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
@bso-intel
[SYCL] Change pattern to reflect the change in sycl-ls (intel/llvm-te…
93ab25d 
…st-suite#256)

In intel#3611, I changed the device type to lower case which was necessary to show the SYCL_DEVICE_FILTER prefix for each device listing from sycl-ls.
It caused several tests failures that expected 'GPU' instead of 'gpu'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanovvlad romanovvlad romanovvlad left review comments

@pvchupin pvchupin pvchupin left review comments

@bader bader bader left review comments

@vladimirlaz vladimirlaz vladimirlaz left review comments

@smaslov-intel smaslov-intel smaslov-intel left review comments

@kbobrovs kbobrovs Awaiting requested review from kbobrovs

@againull againull Awaiting requested review from againull

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bso-intel @vladimirlaz @bader @smaslov-intel @romanovvlad @pvchupin