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.

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

Initial support for ext_oneapi_composite_device. #12178

Merged
merged 33 commits into from
Feb 12, 2024
Merged

Initial support for ext_oneapi_composite_device. #12178

merged 33 commits into from
Feb 12, 2024

Conversation

maarquitos14
Copy link
Contributor

Initial implementation to support sycl_ext_oneapi_composite_device specified in #11846.

Depends on oneapi-src/unified-runtime#1192.

@maarquitos14 maarquitos14 requested review from a team as code owners December 14, 2023 17:03
maarquitos14
maarquitos14 commented Dec 14, 2023
View reviewed changes
sycl/plugins/level_zero/CMakeLists.txt
@@ -4,7 +4,7 @@ if (NOT DEFINED LEVEL_ZERO_LIBRARY OR NOT DEFINED LEVEL_ZERO_INCLUDE_DIR)
message(STATUS "Download Level Zero loader and headers from github.com")

set(LEVEL_ZERO_LOADER_REPO "https://github.com/oneapi-src/level-zero.git")
set(LEVEL_ZERO_LOADER_TAG v1.11.0)
set(LEVEL_ZERO_LOADER_TAG v1.15.1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I tested during development of this patch, I'd say this is not used. It seems that we're now using the equivalent in URT repo.

sycl/plugins/unified_runtime/CMakeLists.txt Outdated
# Author: Maronas, Marcos <marcos.maronas@intel.com>
# Date: Wed Dec 6 03:41:39 2023 -0800
# Initial support for ext_oneapi_composite_device
set(UNIFIED_RUNTIME_TAG e31ef293e1e0a4ed0df351b8b11b03c6dd0967eb)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made to make testing possible during development. It should be updated when the corresponding PR (oneapi-src/unified-runtime#1192) in URT is merged.

aelovikov-intel
aelovikov-intel reviewed Dec 14, 2023
View reviewed changes
sycl/include/sycl/detail/pi.h Outdated Show resolved Hide resolved
sycl/source/detail/composite_device/composite_device.cpp Outdated Show resolved Hide resolved
sycl/source/detail/composite_device/composite_device.cpp Outdated Show resolved Hide resolved
sycl/source/detail/device_impl.cpp Outdated
Comment on lines 572 to 573
if (getBackend() != backend::ext_oneapi_level_zero)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to hardcode that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we don't have to, but we know this extension only works for L0 backend, so we can save the call to PI just by checking this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this, but I can live with that. Please add a comment that this is just a performance optimization though. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 8d68661.

sycl/source/detail/device_info.hpp Outdated Show resolved Hide resolved
sycl/source/detail/device_info.hpp Outdated Show resolved Hide resolved
sycl/source/platform.cpp Outdated
@@ -95,6 +95,37 @@ context platform::ext_oneapi_get_default_context() const {
return detail::createSyclObjFromImpl<context>(It->second);
}

std::vector<device> platform::ext_oneapi_get_composite_devices() const {
// Only some Intel GPU architectures can be composite devices.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to a question earlier, why do we have to know that in the SYCL RT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the comment is just to point out that only GPU architectures can be a composite device, and that's why we only get GPU devices. The Intel part is not really important, but it is informative. I'm open to removing it, if you think it's confusing or does not help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above - I don't like it but I can live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 8d68661.

sycl/source/platform.cpp Outdated Show resolved Hide resolved
sycl/test/extensions/composite_device/device_selector_test.cpp Outdated Show resolved Hide resolved
@maarquitos14
Address code review feedback.
a3bc3e6 
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
@maarquitos14
Temporary update UR repo for testing.
c559036 
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
@maarquitos14
Merge remote-tracking branch 'intel/origin/sycl' into maronas/ext_com…
e5d70ea 
…posite_device
@maarquitos14
Add new Windows ABI symbols.
26f9669 
Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
@maarquitos14
Merge remote-tracking branch 'intel/origin/sycl' into maronas/ext_com…
88b1017 
…posite_device
@maarquitos14
Adjust tests for single-tile cards.
28652e6 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@maarquitos14
Require level-zero for composite device tests.
83bc72b 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@maarquitos14
Update UR commit hash.
031d0d9 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@maarquitos14
Copy link
Contributor Author

@aelovikov-intel I think I addressed all your concerns.

aelovikov-intel
aelovikov-intel reviewed Jan 17, 2024
View reviewed changes
sycl/plugins/unified_runtime/CMakeLists.txt Outdated
Comment on lines 59 to 65
set(UNIFIED_RUNTIME_REPO "https://github.com/maarquitos14/unified-runtime.git")
# commit 75648295df39de3027c989299a0cadb018ea26c8 (HEAD -> maronas/ext_composite_device, origin/maronas/ext_composite_device)
# Merge: a9746c21 c63ad9b2
# Author: Marcos Maronas <marcos.maronas@intel.com>
# Date: Tue Jan 16 09:04:27 2024 -0800
# Merge remote-tracking branch 'intel/origin/main' into maronas/ext_composite_device
set(UNIFIED_RUNTIME_TAG 75648295df39de3027c989299a0cadb018ea26c8)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have to go away before this can be formally approved.

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, definitely, it is just to make sure the CI uses the correct UR version.

sycl/source/detail/composite_device/composite_device.cpp Outdated
Comment on lines 22 to 23
if (std::find(Composites.begin(), Composites.end(), Composite) ==
Composites.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we have an std::hash<device> specialization, so I'd imagine std::set could work here. Feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 8d68661.

sycl/source/detail/composite_device/composite_device.cpp Outdated
Comment on lines 28 to 38
for (const auto &Composite : Composites) {
auto Components = Composite.get_info<info::device::component_devices>();
// Only return composite devices if all of its component devices are
// available.
if (std::all_of(Components.begin(), Components.end(), [&](const device &d) {
return std::find(Devs.begin(), Devs.end(), d) != Devs.end();
})) {
Result.push_back(Composite);
}
}
return Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

That can be an std::copy_if(Composites.begin(), Composites.end(), std::back_inserter{Result}, [](...) { /* predicate */ });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 8d68661.

sycl/source/detail/device_impl.cpp Outdated
Comment on lines 572 to 573
if (getBackend() != backend::ext_oneapi_level_zero)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this, but I can live with that. Please add a comment that this is just a performance optimization though. Same below.

sycl/source/detail/device_info.hpp
sycl::device, ext::oneapi::experimental::info::device::composite_device> {
static sycl::device get(const DeviceImplPtr &Dev) {
if (Dev->getBackend() != backend::ext_oneapi_level_zero)
return {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not throwing an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the extension specification:

The APIs may be called even when using other backends, but they will return an empty list of composite devices.

sycl/test-e2e/CompositeDevice/composite_device.cpp Outdated
Comment on lines 76 to 77
if (!IsL0 || !IsCombined)
assert(CompositeDevs.empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!IsL0 || !IsCombined)
assert(CompositeDevs.empty());
assert(CompositeDevs.empty() || (IsL0 && IsCombined));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8d68661.

sycl/test-e2e/CompositeDevice/composite_device.cpp
Comment on lines +80 to +83
if (std::find(CombinedCompositeDevs.begin(),
CombinedCompositeDevs.end(),
D) == CombinedCompositeDevs.end())
CombinedCompositeDevs.push_back(D);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this check? Devices across platforms can't be the same and I would not expect to have duplicate devices in P.ext_oneapi_get_composite_devices().

Copy link
Contributor Author

@maarquitos14 maarquitos14 Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We're just checking that the following statement from extension spec holds for our implementation:

The free function get_composite_devices returns all of the composite devices across all platforms. The member function platform::ext_oneapi_get_composite_devices returns the composite devices within the given platform.

Particularly, we are checking that the free function returns the composite devices across all platforms, and we are not missing any due to a bug.

sycl/test-e2e/CompositeDevice/composite_device.cpp Outdated
Comment on lines 89 to 90
const auto &D1 = AllCompositeDevs[i];
const auto &D2 = CombinedCompositeDevs[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a guarantee that the order must be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! We don't in this case. We do have to guarantee that several calls to the same function must have the same order, but this is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 8d68661.

sycl/test-e2e/CompositeDevice/composite_device.cpp Outdated
Comment on lines 103 to 104
if (!IsL0 || !IsCombined)
assert(CompositeDevs.empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please combine into a single assert statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8d68661.

sycl/test-e2e/CompositeDevice/composite_device.cpp Outdated
Comment on lines 128 to 130
bool IsL0 = isL0Backend(D.get_backend());
if (!IsL0 || !IsCombined)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very surprised to see this check for IsL0 in a test (it was just a performance optimization in the implementation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 8d68661.

@maarquitos14
Copy link
Contributor Author

@aarongreig conflicts resolved and UR repo and tag updated :)

@maarquitos14
Fix merge issue.
43dc4fa 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@aarongreig
Copy link
Contributor

@maarquitos14 we resolved the CI issues with #12658, that update also pulls in your changes for this PR so you should just be able to resolve the conflict and we can get this merged

@maarquitos14
Mark test as XFAIL.
110b757 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@maarquitos14
Unsupport test for Windows+L0 combination.
6ffce19 
Signed-off-by: Marcos Maronas <marcos.maronas@intel.com>
@steffenlarsen steffenlarsen merged commit 2db1a4f into intel:sycl Feb 12, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlueck gmlueck gmlueck left review comments

@aelovikov-intel aelovikov-intel aelovikov-intel left review comments

@steffenlarsen steffenlarsen steffenlarsen left review comments

@Pennycook Pennycook Pennycook approved these changes

@bso-intel bso-intel bso-intel approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@aarongreig aarongreig aarongreig approved these changes

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

Successfully merging this pull request may close these issues.
8 participants
@maarquitos14 @aarongreig @Pennycook @bso-intel @gmlueck @AlexeySachkov @aelovikov-intel @steffenlarsen