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.

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

[SYCL][ABI-Break] Remove getRawSyclObjImpl() from headers #14585

Merged
merged 13 commits into from
Jul 18, 2024
Merged

[SYCL][ABI-Break] Remove getRawSyclObjImpl() from headers #14585

merged 13 commits into from
Jul 18, 2024

Conversation

ianayl
Copy link
Contributor

@ianayl ianayl commented Jul 16, 2024

As per #10474, I have:

  • Removed getRawSyclObjImpl
  • Changed getSyclObjImpl to return a reference instead

@ianayl ianayl marked this pull request as ready for review July 16, 2024 18:23
@ianayl ianayl requested review from a team as code owners July 16, 2024 18:23
@ianayl ianayl changed the title [SYCL] Remove getRawSyclObjImpl() from headers [SYCL][ABI-Break] Remove getRawSyclObjImpl() from headers Jul 17, 2024
@ianayl
Copy link
Contributor Author

ianayl commented Jul 17, 2024 •
edited

Fails seem to have been addressed by #14589!

It seems that a test case in ninja check-sycl is failing. However, I don't seem to be the only one failing this test case at the exact place. PR's yesterday at around the same time also seem to be facing similar issues:

@ianayl
Copy link
Contributor Author

ianayl commented Jul 17, 2024

Friendly ping @uditagarwal97, this should be ready for review. Thanks!

uditagarwal97
uditagarwal97 approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

SYCL Changes LGTM!

@ianayl
Copy link
Contributor Author

ianayl commented Jul 17, 2024

@intel/bindless-images-reviewers @intel/sycl-graphs-reviewers Friendly ping, this is ready for review. Thanks in advance!

@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Jul 18, 2024 •
edited

@intel/sycl-graphs-reviewers , today is the deadline for the ABI breaking changes, please review ASAP!

@EwanC , @fabiomestre, @reble @Bensuo, @julianmi

fabiomestre
fabiomestre approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@fabiomestre fabiomestre left a comment

Choose a reason for hiding this comment

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

Graph changes LGTM

@ianayl
Copy link
Contributor Author

ianayl commented Jul 18, 2024

Thanks Fabio and Andrei!

@intel/llvm-gatekeepers This should be ready for merging. The Windows E2E test errors seem to be an issue with the runner, but correct me if I'm wrong.

@sarnex
Copy link
Contributor

sarnex commented Jul 18, 2024 •
edited

Yep, fixed in HEAD already.

@sarnex sarnex merged commit 597fc1d into intel:sycl Jul 18, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiomestre fabiomestre fabiomestre approved these changes

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

@DBDuncan DBDuncan DBDuncan 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.

None yet
6 participants
@ianayl @aelovikov-intel @sarnex @fabiomestre @uditagarwal97 @DBDuncan