Skip to content

[SYCL][UR] Update UR loader version #8979

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
Closed
wants to merge 4 commits into from
Closed

[SYCL][UR] Update UR loader version #8979

wants to merge 4 commits into from

Conversation

bmyates
Copy link
Contributor

@bmyates bmyates commented Apr 6, 2023

Signed-off-by: Brandon Yates brandon.yates@intel.com

@bmyates bmyates requested a review from a team as a code owner April 6, 2023 19:24
@bmyates bmyates temporarily deployed to aws April 6, 2023 19:51 — with GitHub Actions Inactive
@bmyates bmyates temporarily deployed to aws April 6, 2023 19:52 — with GitHub Actions Inactive
jandres742
jandres742 reviewed Apr 6, 2023
View reviewed changes
sycl/plugins/unified_runtime/CMakeLists.txt Outdated
@@ -4,7 +4,7 @@ if (NOT DEFINED UNIFIED_RUNTIME_LIBRARY OR NOT DEFINED UNIFIED_RUNTIME_INCLUDE_D
include(FetchContent)

set(UNIFIED_RUNTIME_REPO "https://github.com/oneapi-src/unified-runtime.git")
set(UNIFIED_RUNTIME_TAG d6af758779db6eebdc419fd5e249302f566eb5de)
set(UNIFIED_RUNTIME_TAG 7d22f806c45872dd6326a123915fe3c75be25b0e)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should start having the discipline of updating only stable tags from unified runtime. So in this case, the next weekly tag containing this change. That way, we have always at least a standard practice of what we want to update and we know the changes we are bringing from unified-runtime are stable.

However, I understand the urgency of this fix.

@smaslov-intel : what do you think?

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 we should define validation+promotion process for accepting new UR versions.

Currently I do not think there is any additional validation being done on UR weekly tags. So they are not necessarily more stable than other commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we need improved stability guarantees going forward, what can that UR standalone testing be?
Of course, updating SYCL to use newer UR will additionally run pre-commit SYCL testing.

jandres742
jandres742 reviewed Apr 6, 2023
View reviewed changes
sycl/plugins/unified_runtime/ur/adapters/level_zero/ur_loader_interface.cpp
@@ -86,7 +86,6 @@ UR_DLLEXPORT ur_result_t UR_APICALL urGetEnqueueProcAddrTable(
pDdiTable->pfnMemUnmap = nullptr;
pDdiTable->pfnUSMMemcpy = nullptr;
pDdiTable->pfnUSMPrefetch = nullptr;
pDdiTable->pfnUSMMemAdvise = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed? this is still on UR

https://github.com/oneapi-src/unified-runtime/blob/416b86a2c3de9e7104cf3ba5d11f064eff9c10fe/include/ur_api.h#L5219

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was renamed, this variable is not in table anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmyates could you add the new name, instead of removing it?

https://github.com/oneapi-src/unified-runtime/blob/416b86a2c3de9e7104cf3ba5d11f064eff9c10fe/include/ur_ddi.h#L1042

    ur_pfnEnqueueUSMPrefetch_t pfnUSMPrefetch;
    ur_pfnEnqueueUSMAdvise_t pfnUSMAdvise;
    ur_pfnEnqueueUSMFill2D_t pfnUSMFill2D;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@bmyates bmyates temporarily deployed to aws April 7, 2023 15:37 — with GitHub Actions Inactive
@bmyates bmyates temporarily deployed to aws April 7, 2023 15:38 — with GitHub Actions Inactive
smaslov-intel
smaslov-intel reviewed Apr 7, 2023
View reviewed changes
@smaslov-intel smaslov-intel temporarily deployed to aws April 7, 2023 18:54 — with GitHub Actions Inactive
smaslov-intel
smaslov-intel reviewed Apr 7, 2023
View reviewed changes
@smaslov-intel smaslov-intel temporarily deployed to aws April 7, 2023 23:13 — with GitHub Actions Inactive
@smaslov-intel smaslov-intel temporarily deployed to aws April 7, 2023 23:14 — with GitHub Actions Inactive
@smaslov-intel
Copy link
Contributor

@bader : it looks like this PR did not run any functional testing, did it?

@bader
Copy link
Contributor

bader commented Apr 8, 2023

SYCL end-to-end testing is missing. Please, read this discussion to understand the root cause and suggested solutions.

@smaslov-intel
Copy link
Contributor

SYCL end-to-end testing is missing. Please, read this discussion to understand the root cause and suggested solutions.

I see. @bmyates : please re-base

bmyates added 3 commits April 10, 2023 06:41
@bmyates
[SYCL][UR] Update UR loader version
a27547d 
Signed-off-by: Brandon Yates <brandon.yates@intel.com>
@bmyates
Add pfnUSMAdvise
5ae3a16 
Signed-off-by: Brandon Yates <brandon.yates@intel.com>
@bmyates
formatting
67f2bf8 
Signed-off-by: Brandon Yates <brandon.yates@intel.com>
@bmyates
Copy link
Contributor Author

bmyates commented Apr 10, 2023 •
edited
Loading

@smaslov-intel I rebased onto head of sycl

@bmyates bmyates temporarily deployed to aws April 10, 2023 12:40 — with GitHub Actions Inactive
@bmyates bmyates temporarily deployed to aws April 10, 2023 13:11 — with GitHub Actions Inactive
@bmyates
Copy link
Contributor Author

bmyates commented Apr 10, 2023

This is ready to merge. Only pre-commit failure is unrelated to this PR and I see other PRs have the same test failing:

FAIL: SYCL :: Basic/memory-consumption.cpp

@aelovikov-intel aelovikov-intel mentioned this pull request Apr 10, 2023
Closed
@againull
Copy link
Contributor

RHEL build fails in CI - http://icl-jenkins2.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FBuild_PR_RHEL/detail/Build_PR_RHEL/5489/pipeline/152
That's why PR can't be merged yet.
@bmyates Could you please take a look?

@bmyates
Update to new version to fix rhel build
c27e698 
Signed-off-by: Brandon Yates <brandon.yates@intel.com>
@bmyates bmyates temporarily deployed to aws April 11, 2023 18:04 — with GitHub Actions Inactive
@bmyates bmyates temporarily deployed to aws April 11, 2023 19:58 — with GitHub Actions Inactive
@bmyates
Copy link
Contributor Author

bmyates commented Apr 11, 2023

RHEL build fails in CI - http://icl-jenkins2.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FBuild_PR_RHEL/detail/Build_PR_RHEL/5489/pipeline/152 That's why PR can't be merged yet. @bmyates Could you please take a look?

I think RHEL issue should be fixed now.

pre-merge still failing due to unrelated #9008

@smaslov-intel smaslov-intel reopened this Apr 12, 2023
@smaslov-intel
Copy link
Contributor

rebased in #9053

@smaslov-intel smaslov-intel temporarily deployed to aws April 13, 2023 00:24 — with GitHub Actions Inactive
@smaslov-intel smaslov-intel temporarily deployed to aws April 13, 2023 02:16 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaslov-intel smaslov-intel smaslov-intel approved these changes

@cperkinsintel cperkinsintel cperkinsintel approved these changes

@jandres742 jandres742 jandres742 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.
6 participants
@bmyates @smaslov-intel @bader @againull @cperkinsintel @jandres742