Skip to content
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

Add platform info query to identify its backend adapter #407

Merged
merged 4 commits into from
Apr 7, 2023

Conversation

smaslov-intel
Copy link
Contributor

This allows users to select wanted adapter/platform.

@smaslov-intel
Copy link
Contributor Author

@kbenzie : please review/merge

@smaslov-intel
Copy link
Contributor Author

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@igchor
Copy link
Member

igchor commented Apr 3, 2023

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

@smaslov-intel
Copy link
Contributor Author

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

Thanks @igchor !
But make generate produced tons of changes:

$ git commit -asm "make generate"
[main 8b23094] make generate
12 files changed, 31460 insertions(+), 29538 deletions(-)

Is the correct flow described anywhere?

@smaslov-intel
Copy link
Contributor Author

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

Thanks @igchor ! But make generate produced tons of changes:

$ git commit -asm "make generate"
[main 8b23094] make generate
12 files changed, 31460 insertions(+), 29538 deletions(-)

Is the correct flow described anywhere?

It looks a different clang-format

@igchor
Copy link
Member

igchor commented Apr 3, 2023

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

Thanks @igchor ! But make generate produced tons of changes:

$ git commit -asm "make generate"
[main 8b23094] make generate
12 files changed, 31460 insertions(+), 29538 deletions(-)

Is the correct flow described anywhere?

I think there is only this line in the README: If you've made modifications to the specification, you can also run a custom generate target prior to building.

@igchor
Copy link
Member

igchor commented Apr 3, 2023

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

Thanks @igchor ! But make generate produced tons of changes:

$ git commit -asm "make generate"
[main 8b23094] make generate
12 files changed, 31460 insertions(+), 29538 deletions(-)

Is the correct flow described anywhere?

It looks a different clang-format

Yes, it looks like a wrong clang-format version, but it's strange - we check for specific version in CMake and provide our own rules. What version do you have installed (if any)?

I've pushed regenerated sources formatted with clang-15, the diff is much smaller.

@smaslov-intel
Copy link
Contributor Author

I don't see why there are errors reported in https://github.com/oneapi-src/unified-runtime/actions/runs/4601026787/jobs/8128372658?pr=407

gmake[3]: *** [CMakeFiles/check-generated.dir/build.make:70: CMakeFiles/check-generated] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:1594: CMakeFiles/check-generated.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1601: CMakeFiles/check-generated.dir/rule] Error 2
gmake: *** [Makefile:595: check-generated] Error 2
Error: Process completed with exit code 2.

@smaslov-intel you need to include generated sources in the PR: you should run make generate locally and commit all changes.

Thanks @igchor ! But make generate produced tons of changes:

$ git commit -asm "make generate"
[main 8b23094] make generate
12 files changed, 31460 insertions(+), 29538 deletions(-)

Is the correct flow described anywhere?

It looks a different clang-format

Yes, it looks like a wrong clang-format version, but it's strange - we check for specific version in CMake and provide our own rules. What version do you have installed (if any)?

I've pushed regenerated sources formatted with clang-15, the diff is much smaller.

$ clang-format --version
clang-format version 17.0.0 ([email protected]:smaslov-intel/llvm.git 7719738e32500bff5a61193bfcff9c978ca52e88)

@igchor
Copy link
Member

igchor commented Apr 4, 2023

Ok, I think we didn't actually check for clang-version when doing 'generate': #408

@kbenzie
Copy link
Contributor

kbenzie commented Apr 4, 2023

Is this information not already available via the UR_PLATFORM_INFO_NAME?

I'm wary about the extensibility of an enum.

@smaslov-intel
Copy link
Contributor Author

Is this information not already available via the UR_PLATFORM_INFO_NAME?

I'm wary about the extensibility of an enum.

The UR_PLATFORM_INFO_NAME returns a string, so we'd need a spec how platforms are named so that runtimes can map it back to what they can work with (enums). Rather than having that extra spec and map in each user, I think it is preferable to just add enum here.

@kbenzie
Copy link
Contributor

kbenzie commented Apr 5, 2023

The UR_PLATFORM_INFO_NAME returns a string, so we'd need a spec how platforms are named so that runtimes can map it back to what they can work with (enums). Rather than having that extra spec and map in each user, I think it is preferable to just add enum here.

Okay, while duplicating information isn't ideal I can accept it. Where do you expect this to be used, is this passed through the SYCL RT up to the application?

@smaslov-intel
Copy link
Contributor Author

The UR_PLATFORM_INFO_NAME returns a string, so we'd need a spec how platforms are named so that runtimes can map it back to what they can work with (enums). Rather than having that extra spec and map in each user, I think it is preferable to just add enum here.

Okay, while duplicating information isn't ideal I can accept it. Where do you expect this to be used, is this passed through the SYCL RT up to the application?

The immediate use is for SYCL users to select the underlying backend (via ONEAPI_DEVICE_SELECTOR). Today this selection is via choosing a PI plugin hardcoded in SYCL RT, and with our move to UR we need this new functionality since no longer have visibility into UR adapters.

@kbenzie
Copy link
Contributor

kbenzie commented Apr 5, 2023

The immediate use is for SYCL users to select the underlying backend (via ONEAPI_DEVICE_SELECTOR). Today this selection is via choosing a PI plugin hardcoded in SYCL RT, and with our move to UR we need this new functionality since no longer have visibility into UR adapters.

Ah I see, so its related to #220. When we discussed this in the WG we came to the conclusion this logic would get pulled into UR once PI has been removed. Perhaps when that happens this query would no longer be necessary.

@smaslov-intel
Copy link
Contributor Author

The immediate use is for SYCL users to select the underlying backend (via ONEAPI_DEVICE_SELECTOR). Today this selection is via choosing a PI plugin hardcoded in SYCL RT, and with our move to UR we need this new functionality since no longer have visibility into UR adapters.

Ah I see, so its related to #220. When we discussed this in the WG we came to the conclusion this logic would get pulled into UR once PI has been removed. Perhaps when that happens this query would no longer be necessary.

Unless all the device filtering is moved into UR we will need this interface to stay. Note, SYCL language has intrinsic notion of "backend", so I think this will have to stay forever. E.g. see https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/supported/sycl_ext_oneapi_filter_selector.asciidoc

@smaslov-intel
Copy link
Contributor Author

@kbenzie : can this be merged, please?

Copy link
Contributor

@veselypeta veselypeta left a comment

Choose a reason for hiding this comment

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

LGTM

@kbenzie
Copy link
Contributor

kbenzie commented Apr 7, 2023

@kbenzie : can this be merged, please?

Sure. We aim for two approvals so I was waiting for another reviewer.

Signed-off-by: Sergey V Maslov <[email protected]>
Signed-off-by: Sergey V Maslov <[email protected]>
Signed-off-by: Sergey V Maslov <[email protected]>
@kbenzie kbenzie merged commit 7fa23fc into oneapi-src:main Apr 7, 2023
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.

5 participants