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

[SYCL][UR] Upgrade UR to 2023-09-15 release #11281

Closed
wants to merge 4 commits into from
Closed

Conversation

0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented Sep 22, 2023

No description provided.

@0x12CC 0x12CC requested review from a team as code owners September 22, 2023 20:31
@0x12CC 0x12CC temporarily deployed to WindowsCILock September 22, 2023 20:33 — with GitHub Actions Inactive
Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@@ -31,8 +31,6 @@ UR_DLLEXPORT ur_result_t UR_APICALL urGetGlobalProcAddrTable(
return retVal;
}

pDdiTable->pfnInit = urInit;
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need anymore urInit and urTearDown? here we are removing it from all the tables, so are we removing them from the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe they've already been removed from the specification. The following PR changed them to urLoaderInit and urLoaderTearDown: oneapi-src/unified-runtime#793.

@callumfare, could you please review these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes look correct to me, urInit and urTearDown are now loader-only so should no longer be implemented by the individual adapters.

That said our last attempt to bump the UR commit was reverted in #11227 so I'm not sure if we can bump it again until it's clear whatever issues were occurring are resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @callumfare! I wasn't aware of #11227 but this PR should include those changes as I'm upgrading to a newer release.

@aelovikov-intel, have these CI issues since been resolved? This PR blocks a lot of other work since it's not possible to add new PI APIs without upgrading UR.

Copy link
Contributor

@aelovikov-intel aelovikov-intel Sep 25, 2023

Choose a reason for hiding this comment

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

@aelovikov-intel, have these CI issues since been resolved? This PR blocks a lot of other work since it's not possible to add new PI APIs without upgrading UR.

No. Every time I'm watching the PRs created in my timezone everything goes smoothly. Then overnight/weekend I see our runners in a bad state. I'll repeat - please do NOT create/update any changes for the UR update until we get at least two days with everything green in CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the issues have been resolved with #11306. Please merge latest origin/sycl for re-test.

Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@0x12CC 0x12CC temporarily deployed to WindowsCILock September 25, 2023 14:06 — with GitHub Actions Inactive
Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@0x12CC 0x12CC temporarily deployed to WindowsCILock September 26, 2023 14:12 — with GitHub Actions Inactive
Signed-off-by: Michael Aziz <michael.aziz@intel.com>
@0x12CC 0x12CC temporarily deployed to WindowsCILock September 27, 2023 15:38 — with GitHub Actions Inactive
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

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

CUDA and HIP adapter changes LGTM

@0x12CC
Copy link
Contributor Author

0x12CC commented Sep 29, 2023

This PR isn't needed anymore since there's a new process for updating UR with each adapter change.

@0x12CC 0x12CC closed this Sep 29, 2023
@0x12CC 0x12CC deleted the upgrade_ur branch September 29, 2023 15:20
@aelovikov-intel
Copy link
Contributor

This PR isn't needed anymore since there's a new process for updating UR with each adapter change.

Where is that described? I'd like to understand how that can affect our CI.

@0x12CC
Copy link
Contributor Author

0x12CC commented Sep 29, 2023

This PR isn't needed anymore since there's a new process for updating UR with each adapter change.

Where is that described? I'd like to understand how that can affect our CI.

I'm referring to this process: https://github.com/oneapi-src/unified-runtime/pull/902/files. It seems like each PR that makes an adapter change will update the UR commit used in SYCL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants