-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Define missing feature test macros #4792
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
According to SYCL spec (6.3.3. Feature test macros), vendors are encouraged to group a related set of extensions together into a "feature" and to predefine a feature-test macro when the implementation supports the extensions in that feature. The feature-test macro should have the following form to ensure it is unique: SYCL_EXT_<vendorstring>_<featurename>. The patch defines the following feature test macros: - SYCL_EXT_ONEAPI_ASSERT (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Assert/SYCL_ONEAPI_ASSERT.asciidoc) - SYCL_EXT_ONEAPI_FREE_FUNCTION_QUERIES (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/FreeFunctionQueries/SYCL_INTEL_free_function_queries.asciidoc) - SYCL_EXT_ONEAPI_GROUP_ALGORITHMS (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/GroupAlgorithms/SYCL_INTEL_group_algorithms.asciidoc) - SYCL_EXT_ONEAPI_GROUP_SORT (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/GroupAlgorithms/SYCL_INTEL_group_sort.asciidoc) - SYCL_EXT_ONEAPI_LOCAL_MEMORY (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/LocalMemory/LocalMemory.asciidoc) - SYCL_EXT_ONEAPI_ND_RANGE_REDUCTIONS (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Reduction/Reduction.md) - SYCL_EXT_ONEAPI_DEFAULT_CONTEXT (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/PlatformContext/PlatformContext.adoc) - SYCL_EXT_ONEAPI_USE_PINNED_HOST_MEMORY_PROPERTY (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/UsePinnedMemoryProperty/UsePinnedMemoryPropery.adoc) - SYCL_EXT_ONEAPI_SUB_GROUP (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroup/SYCL_INTEL_sub_group.asciidoc) - SYCL_EXT_INTEL_BITCAST (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Bitcast/SYCL_INTEL_bitcast.asciidoc) - SYCL_EXT_INTEL_DATAFLOW_PIPES (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/DataFlowPipes/data_flow_pipes.asciidoc) - SYCL_EXT_INTEL_EXTENDED_ATOMICS (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/ExtendedAtomics/SYCL_INTEL_extended_atomics.asciidoc) - SYCL_EXT_INTEL_FPGA_DEVICE_SELECTOR (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGASelector.md) - SYCL_EXT_INTEL_FPGA_LSU (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGALsu.md) - SYCL_EXT_INTEL_FPGA_REG (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGAReg.md) - SYCL_EXT_INTEL_KERNEL_ARGS_RESTRICT (for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/KernelRestrictAll/SYCL_INTEL_kernel_restrict_all.asciidoc) The feature macros are defined for documented extensions only.
There were duplicates for the following two feature test macros: - SYCL_EXT_ONEAPI_DEFAULT_CONTEXT (in CL/sycl/platfrom.hpp) - SYCL_EXT_ONEAPI_GROUP_SORT (in sycl/ext/oneapi/group_sort.hpp) The file 'feature_test.hpp.in' is the single point to define all the feature test macros. The user should have ability to test a feature test macro before to include the corresponding header file; therefore, the macro must be defined outside of the header that implements the feature under question.
== Feature Test Macro | ||
|
||
This extension defines the macro `SYCL_EXT_ONEAPI_ASSERT` to `1` to indicate that it is enabled. | ||
|
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.
This specification already defines the feature test macro (see above around line 112). That existing section is the boilerplate we've been using for all new extensions, so please check the other files you've modified to see if they also have an existing feature test macro section.
Also, I think it would be better if all the specifications used the same boilerplate. I'd suggest using the format at line 112 for all extension specifications. If you don't like that existing format, I'm open to discussing alternatives, but I'd like all the specifications to use the same format. The reason we chose to add a table of values is to make it easier to extend these specifications in the future. If we add more functionality in the future, we can easily add a new row to the table (with a new macro value).
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.
@gmlueck Thank you for review and the suggestion. I'll checked all the changed doc files and rewrite the Feature test macro sections using the same boilerplate as at line 112.
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.
@gmlueck Greg, I've fixed the documentation about defined feature test macros using the proposed template.
#define SYCL_EXT_ONEAPI_ENQUEUE_BARRIER 1 | ||
#define SYCL_EXT_ONEAPI_FREE_FUNCTION_QUERIES 1 | ||
#ifndef __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__ |
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.
Why do we have this macro to disable this one extension? None of the other extensions have a similar disable macro. Is there some particular reason why this extension should be different from the others? If not, I'd recommend removing the macro. Note that this macro does not seem to be documented anywhere, so I think it's not part of the extension's API.
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've just used the __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__
macro since the group_algorithm.hpp
header is guarded by the #ifndef __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__
check, so if this macro is defined somewhere, we have no implementation for the extension.
The __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__
macro has been in the group_algorithm.hpp
file since its birthday from commit 8bfa107c2346e304aca0355d9f3945a0233440a4
from @Pennycook co-authored by @rolandschulz and @AlexeySachkov. Colleagues, can you explain, why the macro was used and can/should we remove the #ifndef __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__
guard from both feature_test.hpp.in
and group_algorithm.hpp
files?
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 this is just a case of that extension predating the extension mechanism. I don't remember the specifics, but there was a lot of discussion about giving developers a way to enable and disable individual extensions, and this is what we went with.
We should probably define SYCL_EXT_ONEAPI_GROUP_ALGORITHMS
similar to the other extensions, for the group algorithm interfaces defined in sycl::ext::oneapi::
.
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.
@Pennycook @gmlueck Well, I've added a commit that removes the __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__
guard from both headers. If any objections are there, the commit can just be reverted.
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 that makes sense. @Pennycook, any concerns?
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.
Looks good to me. If this breaks something, we'll find out. :)
We should define 'SYCL_EXT_ONEAPI_GROUP_ALGORITHMS' similar to the other extensions.
The 'Feature test macro' section for all the feature with added macro must be the boilerplate we've been using for all new extensions.
/summary:run |
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.
lgtm
[SYCL] Define missing feature test macros
According to SYCL spec (6.3.3. Feature test macros), vendors are encouraged to
group a related set of extensions together into a "feature" and to predefine
a feature-test macro when the implementation supports the extensions in that
feature. The feature-test macro should have the following form to ensure it is
unique: SYCL_EXT__.
The patch defines the following feature test macros:
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Assert/SYCL_ONEAPI_ASSERT.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/FreeFunctionQueries/SYCL_INTEL_free_function_queries.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/GroupAlgorithms/SYCL_INTEL_group_algorithms.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/GroupAlgorithms/SYCL_INTEL_group_sort.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/LocalMemory/LocalMemory.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Reduction/Reduction.md)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/PlatformContext/PlatformContext.adoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/UsePinnedMemoryProperty/UsePinnedMemoryPropery.adoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroup/SYCL_INTEL_sub_group.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/Bitcast/SYCL_INTEL_bitcast.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/DataFlowPipes/data_flow_pipes.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/ExtendedAtomics/SYCL_INTEL_extended_atomics.asciidoc)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGASelector.md)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGALsu.md)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/IntelFPGA/FPGAReg.md)
(for https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/KernelRestrictAll/SYCL_INTEL_kernel_restrict_all.asciidoc)
The feature macros are defined for documented extensions only and the documentation is updated to mention the corresponding macros.