-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL][ABI Break] Remove "cl" namespace #6518
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
ce49e08
[SYCL][ABI Break] Remove "cl" namespace
aelovikov-intel 64d972a
Merge remote-tracking branch 'origin/sycl' into ns-cl
aelovikov-intel 4e54eb1
Update a comment per code review
aelovikov-intel 678f522
Remove TODOs regarding __host_std versioning
aelovikov-intel 684216f
Align llvm-spirv/lib/SPIRV/SPIRVUtil.cpp change with upstream PR
aelovikov-intel 412ce44
Merge remote-tracking branch 'origin/sycl' into ns-cl
aelovikov-intel f420c43
Delete clang/test/SemaSYCL/intel-fpga-pipeline-ast.cpp
aelovikov-intel 9bcc2d4
Merge remote-tracking branch 'origin/sycl' into ns-cl
aelovikov-intel 48b7958
Merge remote-tracking branch 'origin/sycl' into ns-cl
aelovikov-intel eaf3b39
Add a basic test
aelovikov-intel 930fecd
Merge branch 'sycl' into ns-cl
aelovikov-intel 1265739
Merge remote-tracking branch 'origin/sycl' into ns-cl
aelovikov-intel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you we have SYCL 1.2.1 compatibility tests?
https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:headers-and-namespaces
We should have them in SYCL-CTS and make sure that DPC++ complies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we talking source-compatible or binary-compatible? The former is being done with namespace alias as part of this PR (I might need to add an explicit test for that though), while the latter shouldn't (can't?) be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are talking about source-compatibility. Yes, please, add SYCL-1.2.1 compatibility test to make sure that when <CL/sycl.hpp> is included, we can still use data types defined in
cl::syclnamespace in device code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would need to account for both scopes (here and in Sema) for compatibility right?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove these name based checks. Now that we're modifying namespaces, this isn't future proof at all. We should instead modify
sycl_specialattribute to accept an argument and use that to do these checks where required.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do, but I'd like to hear your reasoning why you think we might need that.
That is way above my comfort level with the FE - I guess I'd need someone from FE to make that part of the change.
Also, IMO, it should be done in a separate patch (either before or after this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the
cl/sycl header is used as @bader mentioned above and users declare types usingcl::sycl::aspect, etc, wouldn't compiler checks likeisDeviceAspectTypefail since it checks hard-coded scope which is nowsycl::_V1::aspect?I can help with that. What is the timeline for this change like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
cl/sycl.hdoesn't change ABI, only introduce namespace alias (https://godbolt.org/z/x17Toq5vY).I don't think there are hard requirement other than the 2023 release, but this PR gets merge conflicts pretty easily and I really wouldn't want it to sit uncommitted for long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Thanks for explaining. I guess this should work then, as long as we don't use older versions of cl/sycl.h