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

Merge changes from multi device compile extension into core spec. #1195

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented Dec 15, 2023

We originally changed this interface a bit from PI: currently these entry points take a context which represents your list of devices. This turned out to cause issues for the level zero adapter so the extension was created to effectively revert back to the PI/CL style interface. This PR reverts the core interface back to PI style, everything takes an explicit device list instead of a context.

LLVM testing intel/llvm#12536

@aarongreig aarongreig force-pushed the aaron/makeDeviceCompileExtCore branch 2 times, most recently from 6c4b652 to 3b9b0d8 Compare December 15, 2023 11:30
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

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

Comparison is base (b66cf9b) 15.46% compared to head (46722a5) 15.37%.
Report is 25 commits behind head on main.

Files Patch % Lines
source/loader/ur_ldrddi.cpp 0.00% 18 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp 0.00% 17 Missing ⚠️
test/conformance/program/urProgramLink.cpp 0.00% 17 Missing ⚠️
test/conformance/program/urProgramBuild.cpp 0.00% 9 Missing ⚠️
test/conformance/program/urProgramCompile.cpp 0.00% 9 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp 0.00% 7 Missing ⚠️
source/loader/ur_libapi.cpp 0.00% 4 Missing ⚠️
source/adapters/null/ur_nullddi.cpp 0.00% 3 Missing ⚠️
test/conformance/testing/include/uur/fixtures.h 0.00% 3 Missing ⚠️
test/conformance/kernel/urKernelCreate.cpp 0.00% 1 Missing ⚠️
... and 5 more

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   15.46%   15.37%   -0.10%     
==========================================
  Files         238      238              
  Lines       33883    33689     -194     
  Branches     3747     3714      -33     
==========================================
- Hits         5239     5178      -61     
+ Misses      28593    28461     -132     
+ Partials       51       50       -1     

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

@aarongreig aarongreig force-pushed the aaron/makeDeviceCompileExtCore branch 9 times, most recently from f26711e to 38c3936 Compare January 26, 2024 10:51
@aarongreig aarongreig marked this pull request as ready for review January 26, 2024 13:02
@aarongreig aarongreig requested review from a team as code owners January 26, 2024 13:03
@JackAKirk
Copy link
Contributor

JackAKirk commented Jan 26, 2024

Can you add a description for this PR please?
I've looked at e.g. #912 and I think I understand that you want to change the interface so it is more flexible and is associating a particular binary with particular devices.

From my experience of CUDA/HIP Outside of MPI which is a completely different mechanism, I am not aware of any multi-device compilation process where the output of a single compilation is to produce different binaries for each of the device. I don't know of any CUDA/HIP documentation on how to deal with this use case.

Therefore, if you could clarify things like:

  • What is the initial motivation for this feature (e.g. give an specific example of a use case that requires such an interface)
  • Which backends is this useful for?
  • Are there any particular considerations for specific backend?

I think that would help with review, allowing me to make a deeper investigation of any potential backend specific issues. Otherwise the only thing I can check is that it doesn't break the standard case where you are just outputting a single binary for all devices.

@aarongreig
Copy link
Contributor Author

The extension was a last minute fix for issues level zero ended up having with a redesign, this basically fully reverts the redesign by merging the extension.

@JackAKirk
Copy link
Contributor

The extension was a last minute fix for issues level zero ended up having with a redesign, this basically fully reverts the redesign by merging the extension.

Is there an accompanying PR in intel/llvm to change this line https://github.com/intel/llvm/blob/f4b4a84b653fb77a9db834d4f588ac347681ea30/sycl/plugins/unified_runtime/pi2ur.hpp#L2068
etc?
It would be nice to check such a PR passes intel/llvm CI.

@aarongreig
Copy link
Contributor Author

..not doing a good job of following my own project's process here

@aarongreig aarongreig force-pushed the aaron/makeDeviceCompileExtCore branch from 38c3936 to 46722a5 Compare January 30, 2024 15:19
Copy link
Contributor

@fabiomestre fabiomestre left a comment

Choose a reason for hiding this comment

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

Looks good to me. Also makes things cleaner in OpenCL 👍

Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for level zero, cleans up things greatly as long as we are not breaking customers that were expecting this behavior.

Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Native CPU LGTM, thank you

Copy link
Contributor

@JackAKirk JackAKirk 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 kbenzie added loader Loader related feature/bug conformance Conformance test suite issues. specification Changes or additions to the specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues labels Apr 10, 2024
@github-actions github-actions bot added the experimental Experimental feature additions/changes/specification label Sep 23, 2024
@github-actions github-actions bot added the sanitizer Sanitizer layer issues/changes/specification label Sep 24, 2024
@aarongreig aarongreig added the ready to merge Added to PR's which are ready to merge label Oct 4, 2024
@aarongreig aarongreig force-pushed the aaron/makeDeviceCompileExtCore branch from e635cac to 1af8237 Compare October 7, 2024 12:47
@aarongreig aarongreig requested a review from a team as a code owner October 7, 2024 13:03
@aarongreig aarongreig removed the ready to merge Added to PR's which are ready to merge label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues. cuda CUDA adapter specific issues experimental Experimental feature additions/changes/specification hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues opencl OpenCL adapter specific issues sanitizer Sanitizer layer issues/changes/specification specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants