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][PI] Add some fixes relevant to adapters second bump PR #11771

Conversation

omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Nov 3, 2023

This PR add some fixes that is needed after adapters branch second bump PR, it needs only to me merged after the second bump PR for adapters branch is merged.

Depends on this PR

@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch 3 times, most recently from 70b08ff to 50bce15 Compare November 3, 2023 14:49
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 50bce15 to dd2a3f4 Compare November 3, 2023 16:05
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from dd2a3f4 to 3e54d64 Compare November 6, 2023 12:27
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 3e54d64 to 3d3b395 Compare November 6, 2023 12:32
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 3d3b395 to 3e047c6 Compare November 7, 2023 10:34
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 3e047c6 to 875c73b Compare November 7, 2023 14:12
@omarahmed1111 omarahmed1111 changed the title Testing for UR adapter branch bump second patch [SYCL][PI] Add some fixes relevant to adapters second bump PR Nov 8, 2023
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch 3 times, most recently from a8aecba to c45bdbf Compare November 8, 2023 10:56
@omarahmed1111 omarahmed1111 marked this pull request as ready for review November 8, 2023 10:59
@omarahmed1111 omarahmed1111 requested review from a team as code owners November 8, 2023 10:59
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 081b046 to 61fdf1f Compare November 10, 2023 12:08
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 61fdf1f to 3941a2d Compare November 10, 2023 15:30
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 3941a2d to 351b253 Compare November 10, 2023 19:44
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from 351b253 to acc1453 Compare November 10, 2023 21:13
@omarahmed1111 omarahmed1111 force-pushed the testing-pr-for-adapter-bump-second-patch branch from acc1453 to a37ad30 Compare November 10, 2023 22:24
@@ -2336,8 +2331,18 @@ inline pi_result piKernelGetInfo(pi_kernel Kernel, pi_kernel_info ParamName,
break;
}
case PI_KERNEL_INFO_NUM_ARGS: {
UrParamName = UR_KERNEL_INFO_NUM_ARGS;
break;
size_t NumArgs = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this change? this seems like something it should go in a completely separate patch so in case there are regressions it is easy to identify.

Copy link
Contributor

Choose a reason for hiding this comment

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

without this one of the fixes we're pulling in causes a regression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes was temporarily until this PR is merged. This changes have been reverted and the PR was rebased on recent SYCL branch.

@omarahmed1111
Copy link
Contributor Author

Due to a windows failure that will be fixed here and to make it easier, we will combine the second and third bump into one here. So closing this for now.

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.

4 participants