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.

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

Compiler option to control SYCL extensions. #806

Open
bader opened this issue Nov 8, 2019 · 16 comments
Open

Compiler option to control SYCL extensions. #806

bader opened this issue Nov 8, 2019 · 16 comments
Assignees
Labels
confirmed enhancement New feature or request

Comments

@bader
Copy link
Contributor

bader commented Nov 8, 2019

Today compiler has only two relevant options to turn on/off SYCL functionality:

Option Description
-sycl-std=<value> SYCL language standard to compile for
-fsycl-unnamed-lambda Allow unnamed SYCL lambda kernels

-sycl-std today supports only one value: "1.2.1" (default) and doesn't really help to restrict functionality to SYCL 1.2.1 version.

I think it would be useful if compiler provides more fine grain control over developed SYCL extensions (e.g. unnamed-lambda #387, CTAD extensions #773, USM #256, etc.).

In addition to that, it would be great to align compiler option names with community way.
E.g. should we keep SYCL specific option -sycl-std or extend existing -std option (e.g. -std supports OpenCL standard versions, hip and cuda values).

Proposal

  1. Extend -sycl-std= values with 1.2.1-ext, which should enable SYCL-1.2.1 functionality with extensions. In addition to that I think this should be a default value similar to how clang enables C++ extensions by default.
  2. Add more options controlling extensions. TBD: define which extensions require compiler option. Some extensions do not require compiler support and can be controlled via define (e.g. Intel sub-groups).
  3. Align options naming scheme with the clang community to simplify upsteaming.

Thoughts?

@bader bader self-assigned this Nov 8, 2019
@AlexeySachkov AlexeySachkov added the enhancement New feature or request label Nov 8, 2019
@AlexeySachkov
Copy link
Contributor

Extend -sycl-std= values with 1.2.1-ext, which should enable SYCL-1.2.1 functionality with extensions. In addition to that I think this should be a default value similar to how clang enables C++ extensions by default.

What if user wants to use a particular set of extension?

Add more options controlling extensions.

Just an idea: -sycl-ext=+ext1,+ext2, similar to OpenCL

TBD: define which extensions require compiler option. Some extensions do not require compiler support and can be controlled via define (e.g. Intel sub-groups).

We could enable automatic passing of defines if extension is enabled (probably this will be easier for some users)

@bader
Copy link
Contributor Author

bader commented Nov 10, 2019

Extend -sycl-std= values with 1.2.1-ext, which should enable SYCL-1.2.1 functionality with extensions. In addition to that I think this should be a default value similar to how clang enables C++ extensions by default.

What if user wants to use a particular set of extension?

I don't see any problems with that. Unused extensions do not affect final results. I think this proposal makes -sycl-std=1.2.1 valuable for code code portability among different SYCL implementations.

@AlexeySachkov
Copy link
Contributor

Extend -sycl-std= values with 1.2.1-ext, which should enable SYCL-1.2.1 functionality with extensions. In addition to that I think this should be a default value similar to how clang enables C++

What if user wants to use a particular set of extension?

I don't see any problems with that. Unused extensions do not affect final results.

I think there might be problems, for example, by SYCL spec you cannot capture a pointer as kernel argument. However, it is legal in USM extension.

My point is that it might be not so clear which exact set of extensions are enabled and user might accidentally use more functionality from extensions than he thought/intended originally.

@rolandschulz
Copy link
Contributor

Maybe we want both? We could add -sycl-std=1.2.1-ext to enable all and -sycl-ext=+ext1,+ext2 to enable (or disable) specific ones. I don't think we only want the option to enable them separately. For users who want all the usability improvements they should have an easy way to enable them all. Otherwise we are adding a new usability problem for novice users.

@asavonic
Copy link
Contributor

Maybe we want both? We could add -sycl-std=1.2.1-ext to enable all and -sycl-ext=+ext1,+ext2 to enable (or disable) specific ones. I don't think we only want the option to enable them separately. For users who want all the usability improvements they should have an easy way to enable them all. Otherwise we are adding a new usability problem for novice users.

For OpenCL we also have -cl-ext=+all and -cl-ext=-all.

@asavonic
Copy link
Contributor

Maybe we want both? We could add -sycl-std=1.2.1-ext to enable all and -sycl-ext=+ext1,+ext2 to enable (or disable) specific ones. I don't think we only want the option to enable them separately. For users who want all the usability improvements they should have an easy way to enable them all. Otherwise we are adding a new usability problem for novice users.

For OpenCL we also have -cl-ext=+all and -cl-ext=-all.

-cl-ext=+all,-cl_khr_fp16 is also supported.

@bader
Copy link
Contributor Author

bader commented Nov 25, 2019

Summary of "SYCL upstream working group" discussion:

  • All agree that we should have flags to turn on/off particular extensions.
  • Flags controlling the same extensions might be set multiple multiple time in the same clang invocation - last one overrides previous flags.
  • Hal: -fsycl-<extension> (e.g. -fsycl-usm) seems reasonable.
  • Use __has_extension to detect if extension is supported in the source code.
  • Hal general I'm okay to enable some extensions by default.
  • Compatibility: Hal, for GCC compatibility we have different names - C++99 + GNU99.
  • The same way we might have driver options compatibility with compute++ driver.
  • Should be aligned with the clang convention to avoid user's hassle with two sets of flags.

@rolandschulz
Copy link
Contributor

If we go with -fsycl-usm rather than -sycl-ext=+usm syntax how do we enable all extensions minus one specific one?

@AlexeySachkov
Copy link
Contributor

If we go with -fsycl-usm rather than -sycl-ext=+usm syntax how do we enable all extensions minus one specific one?

-fsycl-all-extensions -fno-sycl-usm ?

@bader
Copy link
Contributor Author

bader commented Nov 27, 2019

FYI: I'm trying to clarify clang community preference in this thread: http://lists.llvm.org/pipermail/cfe-dev/2019-November/064016.html.

@AlexeySachkov
Copy link
Contributor

I've prepared an initial version of the document describing these options: #1793

@ax3l
Copy link

ax3l commented Jun 6, 2020

Use __has_extension to detect if extension is supported in the source code.

Maybe as __has_sycl_<extension> to avoid conflicts and group all extensions nicely? https://en.cppreference.com/w/cpp/feature_test

@AaronBallman
Copy link
Contributor

Use __has_extension to detect if extension is supported in the source code.

Maybe as __has_sycl_<extension> to avoid conflicts and group all extensions nicely? https://en.cppreference.com/w/cpp/feature_test

Can you explain what conflicts you're concerned about? The point to __has_extension is for the compiler to expose a single feature test point for extensions and the identifiers used by the macro are picked by the implementation, so I'm not certain what conflicts could happen.

Or are you suggesting we go with a C++-style of feature testing where the compiler predefines a bunch of feature testing macros, similar to how __cpp_whatever is a language feature and __cpp_lib_whatever is a library feature? If so, then I'd recommend going with __sycl_whatever and __sycl_lib_whatever for parity.

@keryell
Copy link
Contributor

keryell commented Jan 11, 2021

Use __has_extension to detect if extension is supported in the source code.

Maybe as __has_sycl_<extension> to avoid conflicts and group all extensions nicely? en.cppreference.com/w/cpp/feature_test

Perhaps __sycl_has_extension(some_ext) to be more in the C++ mood?

@AaronBallman
Copy link
Contributor

Use __has_extension to detect if extension is supported in the source code.

Maybe as __has_sycl_<extension> to avoid conflicts and group all extensions nicely? en.cppreference.com/w/cpp/feature_test

Perhaps __sycl_has_extension(some_ext) to be more in the C++ mood?

My feelings are that it doesn't put us more in the C++ mood (__has_extension is a Clang-ism not a C++-ism) and it makes for a slightly worse user experience because portable code now has to add another block of boilerplate for:

#ifndef __sycl_has_extension
#define __sycl_has_extension(x) 0
#endif

on top of any existing boilerplate for handling __has_extension portably.

If we want to be more in the C++ mood, we'd go with __sycl_whatever/__sycl_lib_whatever so users would test like: #if __sycl_has_awesome_sauce >= 202001L or #if __sycl_lib_does_amazing_things.

@gmlueck
Copy link
Contributor

gmlueck commented Jan 11, 2021

Note that the SYCL 2020 provisional specification defines a form of feature-test macros that vendors should use for their extensions. The form includes a string which identifies the vendor, which helps someone reading the code to know which vendor's extension is being used. The working group decided on a all-caps style of feature-test macro to be consistent with other macros in the SYCL spec: SYCL_EXT_<vendorstring>_<extensionname>. For example, a feature defined by the Acme corporation might have a feature-test macro named SYCL_EXT_ACME_FANCYFEATURE. As with other feature-test macro, the value of the macro should be an integer identifying the version of the extension API that the implementation supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants