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] Don't include iostream from sycl.hpp #11373

Open
wants to merge 22 commits into
base: sycl
Choose a base branch
from
Open

[SYCL] Don't include iostream from sycl.hpp #11373

wants to merge 22 commits into from

Conversation

rolandschulz
Copy link
Contributor

No description provided.

@rolandschulz rolandschulz requested review from a team as code owners October 2, 2023 03:59
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 4, 2023 05:25 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 4, 2023 05:49 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 4, 2023 06:17 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

Sign in to view
KseniyaTikhomirova
KseniyaTikhomirova approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

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

LGTM

againull
againull approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

l0-pi LGTM

@rolandschulz
Copy link
Contributor Author

@Alcpz, @joeatodd, @PietroGhg, @uwedolinsky, @stdale-intel: this requires a syclcompat-lib-reviewers review. Can one of you please do that? Thanks!

@rolandschulz
Copy link
Contributor Author

I just noticed #11326. @aelovikov-intel, @steffenlarsen do I need to do the same here (include it unless SYCL2020_CONFORMANT_APIS).

@stdale-intel
Copy link
Contributor

@rolandschulz yes, you will need to hide it under the macro for now, and it will be promoted during ABI breaking cycle

@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 5, 2023 06:46 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 5, 2023 07:13 — with GitHub Actions Inactive
Alcpz
Alcpz approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@Alcpz Alcpz left a comment

Choose a reason for hiding this comment

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

the syclcompat/device.hpp change lgtm

@rolandschulz
Don't include sstream, add test
8a51b31 
also include headers unless SYCL2020_CONFORMANT_APIS
@rolandschulz rolandschulz requested review from a team as code owners October 8, 2023 01:56
bader
bader approved these changes Oct 9, 2023
View reviewed changes
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 10, 2023 03:34 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 10, 2023 04:01 — with GitHub Actions Inactive
@rolandschulz
Copy link
Contributor Author

@KseniyaTikhomirova Do you want to review the changes since your last review? Or is this ready to merge?

KseniyaTikhomirova
KseniyaTikhomirova reviewed Oct 11, 2023
View reviewed changes
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 11, 2023 17:10 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 11, 2023 17:12 — with GitHub Actions Inactive
@aelovikov-intel aelovikov-intel temporarily deployed to WindowsCILock October 11, 2023 17:50 — with GitHub Actions Inactive
@rolandschulz rolandschulz temporarily deployed to WindowsCILock October 11, 2023 17:50 — with GitHub Actions Inactive
@rolandschulz
Copy link
Contributor Author

Ready to merge

aelovikov-intel
aelovikov-intel previously requested changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Blocking for a moment.

@stdale-intel
Copy link
Contributor

@rolandschulz we discussed/reviewed this internally with the architects (@gmlueck / @jbrodman / @bashbaug ) and we do not need to treat this as breaking change, and you can remove the guards if you would like and do not have reason to believe this directly breaks anything.

bader
bader reviewed Oct 11, 2023
View reviewed changes
@rolandschulz
Copy link
Contributor Author

@rolandschulz we discussed/reviewed this internally with the architects (@gmlueck / @jbrodman / @bashbaug ) and we do not need to treat this as breaking change, and you can remove the guards if you would like and do not have reason to believe this directly breaks anything.

Are we also removing the cmath and complex include from sycl.hpp? Otherwise this seems inconsistent. Do you want me to remove them as part of this PR?

@aelovikov-intel
Copy link
Contributor

Do you want me to remove them as part of this PR?

I'll create a separate PR for that.

rolandschulz
rolandschulz commented Oct 12, 2023
View reviewed changes
sycl/plugins/level_zero/tracing.cpp
@@ -16,7 +16,7 @@
#include <layers/zel_tracing_api.h>
#include <ze_api.h>

#include <sycl/detail/iostream_proxy.hpp>
#include <../../source/detail/iostream_proxy.hpp>
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'm pretty sure this is wrong. What's the correct way to share an include between source and plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no - it shouldn't be done. Includes from source aren't shipped.

@aelovikov-intel aelovikov-intel dismissed their stale review October 12, 2023 22:04

not blocking anymore

@againull
Copy link
Contributor

@rolandschulz Could you please resolve merge conflicts.

@github-actions GitHub Actions
Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@KseniyaTikhomirova KseniyaTikhomirova KseniyaTikhomirova approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel left review comments

@Alcpz Alcpz Alcpz approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@againull againull againull approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rolandschulz @stdale-intel @v-klochkov @aelovikov-intel @againull @bader @Alcpz @KseniyaTikhomirova