-
Notifications
You must be signed in to change notification settings - Fork 795
[SYCL][NFC] Remove more explicit "cl::" references #6507
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
Conversation
Previous patch was implemented by splitting off piece of a bigger patch by completely eliminating "cl" namespace and then addressing the local failures. Local testing didn't cover all possible platforms so these occurrences were left untouched. This is a wider application of grep/sed, but it's still not complete as some instances of "cl" namespace references cannot be eliminated in an NFC change (e.g. everything affecting/affected by mangling as in clang or some tools). The reason I'm committing these changes separately is to ease the review of the actual non-NFC PR later.
@@ -27,7 +27,7 @@ NOTE: Khronos(R) is a registered trademark and SYCL(TM) and SPIR(TM) are tradema | |||
|
|||
NOTE: This document is better viewed when rendered as html with asciidoctor. GitHub does not render image icons. | |||
|
|||
This document describes an extension that introduces the `cl::sycl::intel::atomic_ref` class, which exposes additional functionality aligned with the +std::atomic_ref+ class from {cpp}20. |
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 should probably keep references to SYCL 1.2.1 unchanged, it seems cl::sycl is accurate in these cases.
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 wasn't sure how to proceed with those actually. The code that we have in-tree wouldn't have "cl::", so having docs reference it isn't good either.
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.
In other words, are those docs "abstract" and (from the technical perspective) should have lived in the khronos repo, or are they the reflection of our actual implementation?
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.
All docs changes are generally good I think! In supported extensions we should use sycl:: and that will match implementation. I'm just taking about a few very specific references (in deprecated extensions docs) to SYCL 1.2.1 standard where namespaces are cl::sycl in fact. I had to be more specific in my references.
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 agree with this. It would be good to retain cl:: references in specific instances when referring to 1.2.1 specs.
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 had a chat with Pavel yesterday, decided to revert the docs changes under "deprecated/" folder completely. I had to re-focus on another regression, will upload the change once I'll return to this work.
Those are written against SYCL 1.2.1 and our codebase isn't 1.2.1 already.
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.
FE changes LGTM
Previous patch was implemented by splitting off piece of a bigger patch by completely eliminating "cl" namespace and then addressing the local failures. Local testing didn't cover all possible platforms so these occurrences were left untouched. This is a wider application of grep/sed, but it's still not complete as some instances of "cl" namespace references cannot be eliminated in an NFC change (e.g. everything affecting/affected by mangling as in clang or some tools). The reason I'm committing these changes separately is to ease the review of the actual non-NFC PR later.
After intel#6507, the contribution guidelines became confusing.
After #6507, the contribution guidelines became confusing.
Previous patch was implemented by splitting off piece of a bigger patch by
completely eliminating "cl" namespace and then addressing the local failures.
Local testing didn't cover all possible platforms so these occurrences were left
untouched.
This is a wider application of grep/sed, but it's still not complete as some
instances of "cl" namespace references cannot be eliminated in an NFC
change (e.g. everything affecting/affected by mangling as in clang or some
tools). The reason I'm committing these changes separately is to ease the review
of the actual non-NFC PR later.