Initial support for sycl_ext_oneapi_composite_device #1192

maarquitos14 · 2023-12-14T16:38:11Z

Initial implementation to support sycl_ext_oneapi_composite_device specified in intel/llvm#11846.

include/ur_api.h

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

codecov-commenter · 2023-12-18T12:08:21Z

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (c63ad9b) 15.80% compared to head (7564829) 15.79%.

Files	Patch %	Lines
include/ur_print.hpp	0.00%	26 Missing ⚠️
tools/urinfo/urinfo.hpp	0.00%	4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1192      +/-   ##
==========================================
- Coverage   15.80%   15.79%   -0.02%     
==========================================
  Files         223      223              
  Lines       31483    31513      +30     
  Branches     3558     3561       +3     
==========================================
+ Hits         4975     4976       +1     
- Misses      26457    26486      +29     
  Partials       51       51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

GeorgeWeb

HIP and CUDA adapter changes look good to me.

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

source/adapters/level_zero/device.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

…posite_device

maarquitos14 · 2024-01-08T16:14:20Z

@pbalcer @aarongreig I'm unable to reproduce the CI errors in my local machine. Any tips on how to reproduce?

kbenzie · 2024-01-09T14:30:27Z

source/adapters/level_zero/device.cpp

+    if (!isCombinedMode) {
+      ze_device_handle_t RootDev = nullptr;
+      // Query Root Device
+      ZE2UR_CALL(zeDeviceGetRootDevice, (D->ZeDevice, &RootDev));


Is this a new entry point? I'm wondering if the GitHub Actions failures could be due to an older driver being installed or something like that?

@kbenzie It is a new entry point, but as far as I see, the CI uses level zero v1.15.1 which includes this entry. What I'm seeing is that zeDeviceGetRootDevice returns ZE_RESULT_ERROR_UNSUPPORTED_FEATURE under certain circumstances (https://github.com/oneapi-src/level-zero/blob/1685d01497428ca4d8b99200972b64685424d5c9/source/lib/ze_libapi.cpp#L463). The CI is running into this, and that makes ZE2UR_CALL fail in UR_CHECK. I'm not sure how to deal with this, honestly. As of now, I think only PVC should care about this, but I don't think we should have code specific to PVC in UR. Otherwise, we can use ZE_CALL_NOCHECK rather than ZE2UR_CALL and check manually that the error code is either success or unsupported feature. What do you think? Any other suggestion?

Btw, I just checked locally using ZE_CALL_NOCHECK and all the tests pass.

checking the result of ZE_CALL_NOCHECK manually sounds acceptable if there's a safe fallback, I think the adapter does that in other places where stuff might not be supported. @oneapi-src/unified-runtime-level-zero-write should be consulted

Done in 15d2953

kbenzie · 2024-01-09T14:45:31Z

@pbalcer @aarongreig I'm unable to reproduce the CI errors in my local machine. Any tips on how to reproduce?

I believe we test L0 on a machine with an A750.

maarquitos14 · 2024-01-09T14:53:57Z

@pbalcer @aarongreig I'm unable to reproduce the CI errors in my local machine. Any tips on how to reproduce?

I believe we test L0 on a machine with an A750.

Thanks @kbenzie. @pbalcer gave me access to a machine with such hardware. I'm trying to reproduce the issue in that machine now.

maarquitos14 · 2024-01-10T10:39:24Z

@oneapi-src/unified-runtime-level-zero-write @oneapi-src/unified-runtime-native-cpu-write can I have a review please?

source/adapters/level_zero/device.cpp

PietroGhg

Native CPU lgtm, sorry for the late review

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

pbalcer

lgtm, just one minor nit.

source/adapters/level_zero/device.cpp

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

nrspruit

LGTM for level zero

maarquitos14 · 2024-01-11T16:58:30Z

I think this is ready to merge @oneapi-src/unified-runtime-maintain.

aarongreig · 2024-01-11T17:07:44Z

could you update https://github.com/intel/llvm/pull/12178/files#diff-1bf2ed05c0065126d4e2150f0eda4dbd561e5c3625452e96356aadafa1312e6fR64 to point at https://github.com/maarquitos14/unified-runtime/tree/maronas/ext_composite_device so the changes here get tested @maarquitos14 (assuming that PR is going to need to pull the changes in this PR in)

maarquitos14 · 2024-01-11T20:00:49Z

could you update https://github.com/intel/llvm/pull/12178/files#diff-1bf2ed05c0065126d4e2150f0eda4dbd561e5c3625452e96356aadafa1312e6fR64 to point at https://github.com/maarquitos14/unified-runtime/tree/maronas/ext_composite_device so the changes here get tested @maarquitos14 (assuming that PR is going to need to pull the changes in this PR in)

Done in intel/llvm@c559036.

aarongreig · 2024-01-12T15:43:53Z

thanks, you'll need to make sure the CI on intel/llvm side is green as well before we can merge this

…posite_device

maarquitos14 · 2024-01-16T22:07:10Z

thanks, you'll need to make sure the CI on intel/llvm side is green as well before we can merge this

CI on intel/llvm is finally all green. How should we proceed now?

aarongreig · 2024-01-17T12:28:25Z

The ready to merge label represents a queue, when we get to this PR we'll just merge it (assuming no conflicts or whatever come up between now and then) and then ping you on the llvm PR to update the UR tag. You should try to get approvals on the LLVM PR so it doesn't hold up subsequent merges when this PR goes in

…posite_device

Initial implementation to support `sycl_ext_oneapi_composite_device` specified in #11846. Depends on oneapi-src/unified-runtime#1192. --------- Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com> Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>

Initial support for ext_oneapi_composite_device

e31ef29

maarquitos14 requested review from a team as code owners December 14, 2023 16:38

maarquitos14 requested a review from GeorgeWeb December 14, 2023 16:38

maarquitos14 mentioned this pull request Dec 14, 2023

Initial support for ext_oneapi_composite_device. intel/llvm#12178

Merged

aarongreig reviewed Dec 15, 2023

View reviewed changes

include/ur_api.h Outdated Show resolved Hide resolved

maarquitos14 added 2 commits December 15, 2023 13:08

Address code review feedback.

fc661af

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Format code with correct clang-format version.

84a8b39

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

GeorgeWeb approved these changes Dec 18, 2023

View reviewed changes

maarquitos14 and others added 3 commits December 19, 2023 01:10

Update format with correct result from make generate.

586d3ca

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Include new device info queries in corresponding test.

007b24d

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Merge branch 'main' into maronas/ext_composite_device

b2bc62c

pbalcer reviewed Dec 19, 2023

View reviewed changes

source/adapters/level_zero/device.cpp Outdated Show resolved Hide resolved

maarquitos14 and others added 3 commits December 20, 2023 06:36

Stop reading ZE_FLAT_DEVICE_HIERARCHY environment variable.

6e1b825

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

Merge remote-tracking branch 'intel/origin/main' into maronas/ext_com…

a28c6cd

…posite_device

Merge branch 'main' into maronas/ext_composite_device

bc711f4

kbenzie reviewed Jan 9, 2024

View reviewed changes

maarquitos14 and others added 2 commits January 9, 2024 12:39

Manually check error code from zeDeviceGetRootDevice calls.

15d2953

Merge branch 'main' into maronas/ext_composite_device

45e43fd

pbalcer reviewed Jan 10, 2024

View reviewed changes

PietroGhg approved these changes Jan 10, 2024

View reviewed changes

Address code review issues.

a529440

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

pbalcer approved these changes Jan 11, 2024

View reviewed changes

source/adapters/level_zero/device.cpp Outdated Show resolved Hide resolved

Address more code review issues.

a9746c2

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>

nrspruit approved these changes Jan 11, 2024

View reviewed changes

aarongreig added the ready to merge Added to PR's which are ready to merge label Jan 11, 2024

Merge remote-tracking branch 'intel/origin/main' into maronas/ext_com…

7564829

…posite_device

Merge remote-tracking branch 'intel/origin/main' into maronas/ext_com…

5d322ee

…posite_device

aarongreig merged commit b76d907 into oneapi-src:main Feb 7, 2024
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for sycl_ext_oneapi_composite_device #1192

Initial support for sycl_ext_oneapi_composite_device #1192

maarquitos14 commented Dec 14, 2023

codecov-commenter commented Dec 18, 2023 •

edited

Loading

GeorgeWeb left a comment

maarquitos14 commented Jan 8, 2024

kbenzie Jan 9, 2024

maarquitos14 Jan 9, 2024

maarquitos14 Jan 9, 2024

aarongreig Jan 9, 2024

maarquitos14 Jan 10, 2024

kbenzie commented Jan 9, 2024

maarquitos14 commented Jan 9, 2024

maarquitos14 commented Jan 10, 2024

PietroGhg left a comment

pbalcer left a comment

nrspruit left a comment

maarquitos14 commented Jan 11, 2024

aarongreig commented Jan 11, 2024

maarquitos14 commented Jan 11, 2024

aarongreig commented Jan 12, 2024

maarquitos14 commented Jan 16, 2024

aarongreig commented Jan 17, 2024

Initial support for sycl_ext_oneapi_composite_device #1192

Initial support for sycl_ext_oneapi_composite_device #1192

Conversation

maarquitos14 commented Dec 14, 2023

codecov-commenter commented Dec 18, 2023 • edited Loading

Codecov Report

GeorgeWeb left a comment

Choose a reason for hiding this comment

maarquitos14 commented Jan 8, 2024

kbenzie Jan 9, 2024

Choose a reason for hiding this comment

maarquitos14 Jan 9, 2024

Choose a reason for hiding this comment

maarquitos14 Jan 9, 2024

Choose a reason for hiding this comment

aarongreig Jan 9, 2024

Choose a reason for hiding this comment

maarquitos14 Jan 10, 2024

Choose a reason for hiding this comment

kbenzie commented Jan 9, 2024

maarquitos14 commented Jan 9, 2024

maarquitos14 commented Jan 10, 2024

PietroGhg left a comment

Choose a reason for hiding this comment

pbalcer left a comment

Choose a reason for hiding this comment

nrspruit left a comment

Choose a reason for hiding this comment

maarquitos14 commented Jan 11, 2024

aarongreig commented Jan 11, 2024

maarquitos14 commented Jan 11, 2024

aarongreig commented Jan 12, 2024

maarquitos14 commented Jan 16, 2024

aarongreig commented Jan 17, 2024

codecov-commenter commented Dec 18, 2023 •

edited

Loading