-
Notifications
You must be signed in to change notification settings - Fork 113
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
[UR] Improve handling of error cases in urProgramLink #1458
Conversation
9afd1a2
to
783d419
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1458 +/- ##
==========================================
- Coverage 14.82% 12.41% -2.41%
==========================================
Files 250 241 -9
Lines 36220 36293 +73
Branches 4094 4124 +30
==========================================
- Hits 5369 4506 -863
- Misses 30800 31783 +983
+ Partials 51 4 -47 ☔ View full report in Codecov by Sentry. |
da9d876
to
0922999
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HIP and CUDA changes LGTM. Just a few nits.
0922999
to
4a516fb
Compare
There was a problem hiding this 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.
4f47751
to
8da2cf1
Compare
8da2cf1
to
b5a6f3f
Compare
this has merged conflicts, please fix |
Unfortunately the job in intel/llvm#13085 is failing, so I'll need to look into that first when I get the chance. |
I don't see how rebasing this PR requires investigation into the intel/llvm failures. Sure, to get it merged they would need to not fail but that shouldn't be a blocker for rebasing this PR. |
b5a6f3f
to
955a681
Compare
Note that this change includes a specification change: urProgramLink now requires the output parameter to contain either nullptr or some unspecified binary on failure. As well as this change, a number of bugs have been fixed: * The Level Zero adapter now correctly returns `UR_RESULT_ERROR_PROGRAM_LINK_FAILURE` when linking fails, rather than `UR_RESULT_ERROR_UNKNOWN`. * A workaround has been added for some OpenCL devices that return `CL_INVALID_BINARY` rather than `CL_LINK_PROGRAM_FAILURE` on linker failure. * The `phProgram` handle is wrapped in a loader handle by the loader even if an error would be returned. This is required by Level Zero, which outputs a "dummy" program to store the linker log. Conformance tests have also been added.
955a681
to
25c75c0
Compare
Pre-commit PR for oneapi-src/unified-runtime#1458 The wrapper logic for urProgramLink has also been updated to handle the change.
@oneapi-src/unified-runtime-native-cpu-write please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you
Pre-commit PR for oneapi-src/unified-runtime#1458 --------- Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Note that this change includes a specification change: urProgramLink now requires the output parameter to contain either nullptr or some unspecified binary on failure.
As well as this change, a number of bugs have been fixed:
UR_RESULT_ERROR_PROGRAM_LINK_FAILURE
when linking fails, rather thanUR_RESULT_ERROR_UNKNOWN
.CL_INVALID_BINARY
rather thanCL_LINK_PROGRAM_FAILURE
on linker failure.phProgram
handle is wrapped in a loader handle by the loader even if an error would be returned. This is required by Level Zero, which outputs a "dummy" program to store the linker log.Conformance tests have also been added.