-
Notifications
You must be signed in to change notification settings - Fork 787
Bump UR tag to include PrintTrace fix #14728
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
Conversation
9766a9f
to
eb0b0d2
Compare
eb0b0d2
to
7aa3d88
Compare
@intel/dpcpp-esimd-reviewers Could we have a review on this when available ASAP to merge it? Thanks! |
# commit bc1a28ede0df7f837047b632e00437587672c134 | ||
# Author: Omar Ahmed <[email protected]> | ||
# Date: Mon Jul 29 16:44:58 2024 +0100 | ||
# Merge pull request #1819 from DBDuncan/sean/rename-interop-to-external | ||
# [Bindless][Exp] Rename interop related structs/funcs with "external" | ||
set(UNIFIED_RUNTIME_TAG bc1a28ede0df7f837047b632e00437587672c134) | ||
set(UNIFIED_RUNTIME_TAG a2a053de43b8488a571ec026512a98b178ca06e4) |
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.
This was merged, so could be updated to oneapi-src/unified-runtime@3e762e0. This PR would then include all the updates from #14848.
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.
Updated
ec0f57a
to
e2bb05c
Compare
The bump for this PR will be included as part of this PR, so we could remove the UR bump here. |
The test changes (other than two Plugin tests) are all needed at the same time as the bump, because the UR changes cause the test output to be formatted differently on L0. So that PR would need to temporarily disable those tests on some configurations, and then reenable them here. It might be easier to merge this separately. |
Makes sense. This PR is nearly finished with CI anyway. I think this should be merged first, followed by #14701. @intel/llvm-reviewers-runtime please review. |
@intel/llvm-reviewers-runtime can you please approve? This PR only has test and UR related changes. The correct reviewer team should be @intel/unified-runtime-reviewers, but this PR was created before the |
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
@@ -1,6 +1,5 @@ | |||
// REQUIRES: gpu, level_zero | |||
// TODO: Reenable, see https://github.com/intel/llvm/issues/14721 | |||
// UNSUPPORTED: ze_debug, windows, linux | |||
// UNSUPPORTED: ze_debug |
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.
Not blocking the merge, but should this eventually be enabled? If so, please, create another issue for this.
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.
I think it was always like that, even before PI removal.
See here.
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.
@steffenlarsen and/or @lbushi25 are working on an ongoing effort to review all XFAIL
and UNSUPPORTED
directives in our e2e tests to add an explicit comment that those are either expected to be there due to our implementation status/device capabilities, or that those should be fixed as part of this or that tracker.
@intel/llvm-gatekeepers please merge |
@@ -178,15 +178,15 @@ int main() { | |||
} | |||
|
|||
// launch of Gen kernel | |||
// CHECK:---> urKernelCreate( |
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.
This is not to block the PR, but my opinion is that we shouldn't have tests relying on tracing in e2e
tests. Those should instead be refactored to be unit-tests. I've highlighted this already in PI removal PR and I would be happy to contribute to such rewrite myself (if only I had some time for it, sigh).
Tests oneapi-src/unified-runtime#1884
Fixes #14704
Fixes #14721