Initial version of the proposal of SYCL/DPC++ language features control #1793
Conversation
|
|
||
| **TODO**: do we need to specify which exact version is implied by which | ||
| `-sycl-std` value? AFAIK, SYCL spec only specifies minimum required C++ | ||
| version, which is C++11 and we set C++17 in our implementation |
There was a problem hiding this comment.
We should at least specify what is the minimum version that will be set, this compiler flag will be used by other implementations that may have different behaviors.
It should state that if the user tries to set a CPP version that is less than the minimum required by the spec it will fail to compile.
This is more important for the next SYCL version which will be based on C++17
There was a problem hiding this comment.
I actually like a suggestion from @rolandschulz:
-sycl-std doesn't imply any particular C++ standard version to be set and default version is selected (whichever default is for clang). If the default doesn't work as expected, user should use a combination of -sycl-std + -std to specify both standards.
If some combination of -sycl-std and -std is not supported, then user will see an error: this could be done by some macro checks within SYCL headers, for example
-std=sycl-1.2.1 in turn would imply some C++ standard to be configured. Since the spec doesn't tie us to a particular version and only specifies some minimum version, this option would imply some C++ standard, which is at least the minimum version according to the spec.
What do you think?
There was a problem hiding this comment.
I think it makes sense. Just a question: What happens if -sycl-std=2020 is used on a clang version that doesn't support C++17 ? I think that should fail. Obviously it cannot happen on DPCPP, but it would be good if at least the flag behavior is documented for all cases so other implementations can do the same.
There was a problem hiding this comment.
Just a question: What happens if -sycl-std=2020 is used on a clang version that doesn't support C++17 ? I think that should fail.
I don't know the exact wording which will be used in the upcoming spec, so I cannot say for sure. If C++17 would be declared as minimum required version, then yes, it should fail.
I'm not sure that we need to add special diagnostic into the compiler for that - probably some compilation error from SYCL headers would be enough (can be easily achieved by #ifdef + #error)
it would be good if at least the flag behavior is documented for all cases so other implementations can do the same.
Ok, I will add this
There was a problem hiding this comment.
Applied mentioned suggestion and added a note about incompatible SYCL/DPC++ and C++ versions in e702e9b
|
|
||
| - `dpcpp-0.7`: corresponds to DPC++ 0.7. | ||
|
|
||
| Basically, `-sycl-std=dpcpp-0.7` implies support for SYCL 1.2.1 specification, |
There was a problem hiding this comment.
So every version of DPC++ will have a different set of extensions? Where is the documentation for those extensions? Note that if the extensions evolve on future versions of DPC++ and the documents in the repo are updated, there may be no trace of the old behavior. Unless this is referring to the oneAPI spec version? In which case I think the name should be -sycl-std=oneapi-0.7 to make that clear
There was a problem hiding this comment.
So every version of DPC++ will have a different set of extensions?
It would be good to see comments from @jbrodman here.
Personally, I don't know for sure, but I would expect that as spec evolves, it might include either more extensions that previous version, or less if SYCL version which is used as base was updated.
Where is the documentation for those extensions?
The list of extensions is documented in oneAPI spec and it contains links to specifications for each particular extension
Note that if the extensions evolve on future versions of DPC++ and the documents in the repo are updated, there may be no trace of the old behavior.
Good point. My understand that mitigation here is the following:
- oneAPI spec should probably document which particular revisions of extensions are required to be supported
- According to SYCL_INTEL_extension_api, there should be a way to check (approximately), which revision of extension is enabled
- When extension is updated, any significant change of it must lead to revision number and history of changes to be updated to provide some kind of diff about what changed and how
- In case of very significant changes to the extension, it is probably better to prepare a new one and deprecate the old one, rather than update revision number
Unless this is referring to the oneAPI spec version? In which case I think the name should be -sycl-std=oneapi-0.7 to make that clear
This indeed refers to oneAPI spec version, but If I understand correctly, the whole oneAPI is wider than just DPC++ and here, in compiler, we are talking about DPC++. I guess it is worth to add some note that DPC++ version is the same as oneAPI version or something like this
There was a problem hiding this comment.
I guess it is worth to add some note that DPC++ version is the same as oneAPI version or something like this
Added such clarification in e702e9b
| [oneAPI Specification]: https://spec.oneapi.com/ | ||
|
|
||
| Note: it is possible to change C++ version independently of SYCL/DPC++ standard | ||
| version if that is needed: `-sycl-std=1.2.1 -std=c++14`, for example. |
There was a problem hiding this comment.
| version if that is needed: `-sycl-std=1.2.1 -std=c++14`, for example. | |
| version if that is needed: `-sycl-std=1.2.1 -std=c++14`, for example - as long as that version is equal or above the minimum required by the SYCL standard version. |
There was a problem hiding this comment.
I reworded this a bit in e702e9b - could please check that new wording reflects your comment?
There was a problem hiding this comment.
then it is expected to see compilation errors. Its unclear whether this is just going to be some random C++ compilation error because using the wrong interface (e.g., lack of CTAD would cause strange template errors) or there will be a specific "This version of C++ is not supported by this SYCL version". I would rather have the explicit error message, if possible.
| **TODO**. Please let me know if we don't want to go with this approach at all. | ||
| Note on motivation: clang accepts OpenCL standards via `-std` option, | ||
| as well as `cuda` and `hip`. So, values mentioned above were added for | ||
| consistency and for providing some additional aliases. |
There was a problem hiding this comment.
I think it makes sense to follow everyone else's approach, easier for users.
There was a problem hiding this comment.
Thanks, removed that TODO in 1d858ed
| clear error that extension name is invalid, user will get bunch of errors that | ||
| particular types/methods or functions are not available. For example: | ||
| `-DSYCL_INTEL_SUBGRUOP_ALGORITHMS_ENABLE` - note the typo. | ||
|
|
There was a problem hiding this comment.
I think its better if all extensions are enabled via flags. I don't think this should prevent users from doing it manually, but having a consistent way of enabling/disabling them would be preferable.
There was a problem hiding this comment.
I don't think this should prevent users from doing it manually, but having a consistent way of enabling/disabling them would be preferable.
Exactly. I guess that internally modifications to header files will be guarded by #ifdefs and macro will be set automatically by the compiler if corresponding extension is enabled via flag. However, this shouldn't block users from manual usage of that macro to control header files content
|
|
||
| ## Controlling SYCL/DPC++ language extensions | ||
|
|
||
| Both SYCL and DPC++ has several extensions or proposals about how to expand |
There was a problem hiding this comment.
My understanding is that DPC++ is a set of extensions over SYCL 1.2.1 , so defining DPC++ as the -sycl-std version should simply be enabling a set of extensions on top of the sycl-2015 profile?
There was a problem hiding this comment.
so defining DPC++ as the -sycl-std version should simply be enabling a set of extensions on top of the sycl-2015 profile?
Yes, I noted it here: https://github.com/intel/llvm/pull/1793/files#diff-e5315c99e6f09a6f7b056a184ea4af5fR32
However, if I understand correctly, some DPC++ extensions might still be optional, at least for some device types. You can find table with full list of extension here. For example, sub-groups are not required to be supported on FPGA and someone might want to disable that extension explicitly it target platform is FPGA: even in DPC++ mode, just to avoid accidental use of it
There was a problem hiding this comment.
Some DPC++ extensions could become SYCL optional features. The sycl-2020 profile could enable some optional features if the device supports them. So any DPC++ extension that has been included on the SYCL 2020 document but it is only supported on some devices will be enabled when -sycl-std=sycl2020 is set. The -sycl-std=dpcpp-0.XX profile would only add those extensions not yet on SYCL specification document. I guess is a larger discussion than this PR.
| Cons: | ||
| - Seems like a significant amount of new flags coming to the compiler | ||
| - Harder to prototype and implement new extensions | ||
| - What about header-only extensions? Bunch of compiler options which are only |
There was a problem hiding this comment.
The current list of extensions may be large, but once the next SYCL version is out, most of them will simply be the new "normal" under sycl-2020. I don't think is a huge burden for users to set them individually. Until there is a complete 2020 implementation, using the -sycl-std=dpcpp-07 should set all relevant not-target specific extensions, so the user should only have to add those extensions that are target specific. That list should not be that large, unless I am mistaken, so having separate flags should be fine.
There was a problem hiding this comment.
The current list of extensions may be large, but once the next SYCL version is out, most of them will simply be the new "normal" under sycl-2020. I don't think is a huge burden for users to set them individually.
This is probably not only about user experience, but also about amount of changes in the compiler: for how long we are going to support older standard with all its extensions and corresponding flags?
There was a problem hiding this comment.
Depending on how much of dpc++ gets upstreamed to llvm, this may be a long time. I would assume, however, a large part of the changes and extensions will stabilize. There is a large gap between SYCL 1.2.1 and DPC++ that will be much smaller after SYCL 2020.
| value in particular form. How can we automatically put any meaningful value | ||
| in there without having predefined list of known extensions? Do we need to | ||
| extend the format so user can specify which version of the extension is | ||
| needed (`-sycl-ext=+EXTENSION_NAME1=123`)? |
There was a problem hiding this comment.
Having a multiple-option flag can be problematic for build systems, or for users that have scripts to generate command line flags. Is there an example of this type of flag currently in clang/llvm ?
There was a problem hiding this comment.
Having a multiple-option flag can be problematic for build systems, or for users that have scripts to generate command line flags.
Why so? This is just the same strings concatenation, just using comma instead of space as a separator. Moreover, the intent is to allow to pass -sycl-ext flag several times: the final list of extensions will be calculated as sum of values for each occurrence of the flag.
Is there an example of this type of flag currently in clang/llvm ?
Honestly, I'm only aware of -cl-std, which is not even available to end user and only accessible via -Xclang or in -cc1 mode.
There are several flags talking about extensions, but they seem to enable some unspecified set of them, rather than allowing to precisely select desired extensions: -fms-extensions, -fno-borland-extensions, -fapplication-extensions, -menable-experimental-extensions
There was a problem hiding this comment.
Documented my intent about several occurrences of -sycl-ext in 349ec2d
There was a problem hiding this comment.
Trying to use this combination of -sycl-ext accurately on a build system like CMake could be difficult. I don't know enough about it to answer it, maybe I am just being overly cautious.
Moreover, the intent is to allow to pass -sycl-ext flag several times: the final list of extensions will be calculated as sum of values for each occurrence of the flag.
This can be confusing. Usually on the compiler command line, the latest command is the winner. However, now you have to go and find the entire command line and count all occurences to figure out which extension is enabled and which one is disabled? When you use it directly on the command line may be fine, but when you use this from some generator (again, like CMake, but may as well be some python script), you may end up having some unexpected outcomes.
| **TODO**: update table with supported extensions with identifiers (macro or | ||
| compiler options), which should be used to enable/disable them. |
There was a problem hiding this comment.
Sounds great, maybe the macro checks could read __has_sycl_<extension> for grouping/readability with other __has_<feature> and __has_cpp_<feature> options in the ISO C++ standard? https://en.cppreference.com/w/cpp/feature_test
+1 for a table to find extension flag names, macros and reference.
There was a problem hiding this comment.
BTW, table with extensions and references to corresponding specs already exists: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/README.md
Will be extended with additional information as we finalize this proposal.
I'm not exactly sure about __has_ part: If I understand correctly, there are only two __has_ things in C++: __has_cpp_attribute and __has_include (not sure about this one).
The rest looks like __cpp_feature for language features which requires compiler support and _cpp_lib_feature for library features, which doesn't require compiler support. According to recomendations, if some feature requires both compiler and library support, compiler guard should contain impl substring and user should only check for library guard. I.e. __cpp_impl_coroutine and __cpp_lib_coroutine - the latter won't be available if the former is not available.
Personally, I think that it would be good to adapt that to SYCL. The 1.2.1 spec doesn't say anything about extensions, but I hope that the next major version will do. We have corresponding proposal, which mentions feature test macro, but with a bit different wording: SYCL_KHR_extension_name or SYCL_EXT_vendor_extension_name
Changes: - documented that -sycl-std doesn't imply C++ standard - documented that -std=sycl/dpcpp implies some C++ standard - added clarification about incompatible versions of SYCL/DPC++ and C++
As there seem to be no objections agains this
| One more way to specify SYCL/DPC++ standard version is to use a general clang | ||
| option, which allows to specify language standard to compile for. | ||
|
|
||
| Supported values (besides listed in clang documentation/help): |
There was a problem hiding this comment.
Rather than listing the values again, it might be better to just refer to the values above. That way we don't need to add every future value twice.
There was a problem hiding this comment.
This is done on purpose, as I don't think that each value from -sycl-std fits -std option. I.e. with -sycl-std option it is clear that we are talking about SYCL (or about DPC++) standard, while for -std it might be as well CUDA, C++, HIP, OpenCL, etc. For example, 2017 might be confusing: is it C++17 or SYCL 1.2.1?
There was a problem hiding this comment.
Yes only options starting with sycl- or dpcpp- should work for with -std.
Reworded some parts according to suggestions. Clarified which C++ standard is implied when SYCL/DPC++ version is set via `-std` flag
Clarified behavior when flag was set several times or the same extension name was specified several times.
No description provided.