Skip to content

Initial version of the proposal of SYCL/DPC++ language features control #1793

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

Closed
wants to merge 7 commits into from
Closed

Initial version of the proposal of SYCL/DPC++ language features control #1793

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3254bcd
Initial version of the proposal of SYCL/DPC++ language features control
AlexeySachkov Jun 1, 2020
8829abc
Fix formatting
AlexeySachkov Jun 9, 2020
e702e9b
Reword -sycl-std and -std descriptions
AlexeySachkov Jun 10, 2020
39c89f3
Apply suggestions
AlexeySachkov Jun 10, 2020
1d858ed
Remove TODO about extendind -std option
AlexeySachkov Jun 10, 2020
385b6a4
Apply suggestions
AlexeySachkov Jun 11, 2020
349ec2d
Update description of `-sycl-ext` option
AlexeySachkov Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 195 additions & 0 deletions sycl/doc/ControllingSYCLLanguageFeatures.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
# Controlling SYCL/DPC++ language features

This documents aims to describe a mechanism which is used to control (i.e.
enable or disable) different SYCL/DPC++ language and library features, like
version of SYCL/DPC++ standard to use, set of extensions to use, etc.

## Controlling SYCL/DPC++ standard version

In order to control the language standard to compile for, the following options
can be used:

`-sycl-std=<value>`, `--sycl-std=<value>`, `--sycl-std <value>`

SYCL language standard to compile for.

Possible values:

- `2017`, '121', `1.2.1`, `sycl-1.2.1`: corresponds to SYCL 1.2.1
specification, see [SYCL Registry] for more details and the specification
text.

Note: setting `-sycl-std` option automatically implies a particular C++
standard version to be set.

**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
Copy link

@Ruyk Ruyk Jun 3, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 4, 2020

Choose a reason for hiding this comment

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

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?

Copy link

@Ruyk Ruyk Jun 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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,
Copy link

@Ruyk Ruyk Jun 3, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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

bunch of extension enabled and `-std=c++17`

See [oneAPI Specification] for more details and the specification text.

[SYCL Registry]: https://www.khronos.org/registry/SYCL/
[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.
Copy link

@Ruyk Ruyk Jun 3, 2020

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

I reworded this a bit in e702e9b - could please check that new wording reflects your comment?

Copy link

@Ruyk Ruyk Jun 11, 2020

Choose a reason for hiding this comment

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

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.


`-std=<value>`, `--std=<value>`, `--std <value>`

One more way to specify SYCL/DPC++ standard version is to use general clang
option which allows to specify language standard to compile for.

Supported values (besides listed in clang documentation/help):
Copy link
Contributor

@rolandschulz rolandschulz Jun 10, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@rolandschulz rolandschulz Jun 11, 2020

Choose a reason for hiding this comment

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

Yes only options starting with sycl- or dpcpp- should work for with -std.


- `sycl-1.2.1`, `sycl-2017`: corresponds to `-sycl-std=1.2.1`
- `dpcpp-0.7`: corresponds to `-sycl-std=dpcpp-0.7`

Please note that if you specify `-std` flag several times, only the latest
value takes effect. This means, that if you want to specifiy particular C++
standard version instead of some default one implied by SYCL/DPC++ standard,
you have to use two separate options: `-sycl-std` and `-std`.

**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.
Copy link

@Ruyk Ruyk Jun 3, 2020

Choose a reason for hiding this comment

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

I think it makes sense to follow everyone else's approach, easier for users.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

Thanks, removed that TODO in 1d858ed


## Controlling SYCL/DPC++ language extensions

Both SYCL and DPC++ has several extensions or proposals about how to expand
Copy link

@Ruyk Ruyk Jun 4, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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

Copy link

@Ruyk Ruyk Jun 11, 2020

Choose a reason for hiding this comment

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

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.

standard with new features, which might be vendor- or device-specific.

Each extension can be separately enabled or disabled depending on user
preferences. List of extensions enabled by default is controlled by SYCL/DPC++
standard version.

Enabling/disabling extensions is done via single `-sycl-ext` option. It accepts
comma-separated list of extension names prefixed with `+` or `-` to indicate
whether particular extension should be enabled or disabled.

Example: `-sycl-ext=+EXTENSION_NAME1,-EXTENSION_NAME2` - this option specifies
that `EXTENSION_NAME1` extension should be enabled and `EXTENSION_NAME2` should
be disabled.

When particular extension is enabled, the compiler automatically defines a macro
with the same name, i.e. if `-sycl-ext=EXTENSION_NAME1` command line option was
specified, then `EXTENSION_NAME1` macro will be defined.

**TODO**: update table with supported extensions with identifiers (macro or
compiler options), which should be used to enable/disable them.
Comment on lines +93 to +94
Copy link

@ax3l ax3l Jun 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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


### Materials for discussion

Details of controlling SYCL/DPC++ language extensions for sure are not settled
down yet and there are few questions and different ways of answering them.

Information below is provided to highlight main questions and pros and cons of
different solutions to select the best one.

#### Macro vs. compiler option for header-only extensions

There are extensions/proposals, which doesn't require any specific changes
in the compiler or underlying components, but just define some helpers or
sugar in the API and therefore, can be purely implemented in header files,
without even changing SYCL/DPC++ runtime. For example,
[SYCL_INTEL_device_specific_kernel_queries]

[SYCL_INTEL_device_specific_kernel_queries]: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/DeviceSpecificKernelQueries/SYCL_INTEL_device_specific_kernel_queries.asciidoc

On the one hand, it shouldn't be necessary to modify the compiler in order to
enable/disable some new methods or types defined in some header file. It should
be enough to guard the content of a such file with some `#ifdef EXTENSION_NAME`
and user is expected to pass `-DEXTENSION_NAME` to compiler in order to use such
extension.

So, the main pros here is that no changes to the compiler is needed at all.

However, there are several downsides of that approach:

- According to [Predefined macros] section from [SYCL_INTEL_extension_api]
proposal, if toolchain supports extension, it should _automatically_ define
corresponding macro. So, if we require from user to specify the macro for an
extension, it contradicts with the current proposal of extensions mechanism in
SYCL.

- It is inconsistent for users (and they likely don't really care about how
particular extension is designed internally) why some of them are enabled
via some compiler flag and another ones are enabled via macro. Also,
implementation of every extension might be changed in the future as extension
and the whole SYCL implementation evolves over time.

- It is easy to make a typo in some extension name and instead of having one
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.

Copy link

@Ruyk Ruyk Jun 3, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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


[SYCL_INTEL_extension_api]: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/ExtensionMechanism/SYCL_INTEL_extension_api.asciidoc
[Predefined macros]: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/ExtensionMechanism/SYCL_INTEL_extension_api.asciidoc#predefined-macros

#### -sycl-ext vs. -fsycl-extension-name/-fno-sycl-extension-name

Another one way to enable/disable extensions is to provide separate option
for each extensions.

There are pros and cons of both approaches:

##### Single flag, i.e. -sycl-ext

Pros:
- We don't have to hardcode list of known extensions and update the compiler
for each new extension we have prepared: `-cl-std` works in this manner

- This simplifies prototyping of new extensions, especially if they are
header-only

- This allows to easily treat header-only extensions in the same way as
actual compiler extension and change the implementation without any
updates in compiler/toolchain interface (command line options)

- Enabling/disabling extension via this option could automatically define
corresponding macro, which can be used by header-only extensions to hide
new classes/APIs and other stuff introduced by the extension to avoid it
being accidentally used

Cons:
- Without list of known extensions, we cannot emit proper diagnostic that some
unknown extension was enabled just because of the typo in its spelling

- Potentially, we could introduce one more option, which will allow to check
that list of enabled/disabled extensions doesn't contain anything unknown,
but this again means change of the compiler for each even header-only
extension

- According to [Predefined macros] section from [SYCL_INTEL_extension_api]
proposal, extension-specific macro should not only be defined, but also has a
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`)?
Copy link

@Ruyk Ruyk Jun 4, 2020

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 11, 2020

Choose a reason for hiding this comment

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

Documented my intent about several occurrences of -sycl-ext in 349ec2d

Copy link

@Ruyk Ruyk Jun 11, 2020

Choose a reason for hiding this comment

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

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.


##### Separate flag for each extension

Generic form of command line option to control SYCL/DPC++ language extension
is `-f[no-]sycl-extension-name`. For example, `-fsycl-usm` flag enables support
for [USM] extension, `-fno-sycl-usm` disables it.

[USM]: https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/USM/USM.adoc

Flags controlling the same extension might be set multiple times in the same
compiler invocation: the last one overrides previous flag.

Pros:
- We will automatically get a diagnostic if user made a typo in an option name,
which corresponds to a particular extension

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
Copy link

@Ruyk Ruyk Jun 4, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 10, 2020

Choose a reason for hiding this comment

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

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?

Copy link

@Ruyk Ruyk Jun 11, 2020

Choose a reason for hiding this comment

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

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.

used to define a macro doesn't look good