Skip to content

Comments

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

Closed
bmyates wants to merge 4 commits intointel:syclfrom
bmyates:fix_ur_symbols
Closed

[SYCL][UR] Update UR loader version#8979
bmyates wants to merge 4 commits intointel:syclfrom
bmyates:fix_ur_symbols

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
Copy link
Contributor

@jandres742 jandres742 Apr 6, 2023

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

@bmyates bmyates Apr 6, 2023

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

@smaslov-intel smaslov-intel Apr 6, 2023

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 Outdated
Copy link
Contributor

@jandres742 jandres742 Apr 6, 2023

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

@bmyates bmyates Apr 6, 2023

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

@jandres742 jandres742 Apr 6, 2023

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

@bmyates bmyates Apr 7, 2023

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

smaslov-intel commented Apr 7, 2023

@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

smaslov-intel commented Apr 8, 2023

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

againull commented Apr 10, 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?

@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

smaslov-intel commented Apr 12, 2023

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

@cperkinsintel cperkinsintel cperkinsintel approved these changes

+2 more reviewers

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

@jandres742 jandres742 jandres742 approved these changes

Reviewers whose approvals may not affect merge requirements

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