[SYCL] Adds compile-time properties #4976

steffenlarsen · 2021-11-17T15:14:01Z

Implements the compile-time property list extension adding the sycl::ext::oneapi::properties and sycl::ext::oneapi::property_value classes. These allow for compile-time inspection of properties passed to SYCL objects. These changes do not add support for the new property lists to any existing SYCL classes however.

Implements the compile-time property list extension (https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/PropertyList/SYCL_EXT_ONEAPI_property_list.asciidoc) adding the sycl::ext::oneapi::property_list and sycl::ext::oneapi::property_value classes. These allow for compile-time inspection of properties passed to SYCL objects. These changes do not add support for the new property lists to any existing SYCL classes however. Signed-off-by: Steffen Larsen <[email protected]>

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen · 2021-11-17T17:58:10Z

@gmlueck | @rolandschulz | @Pennycook - It seems that MSVC is complaining because get_property has both static and non-static overloads. It appears to be a problem we have had with accessor_property_list before as well: https://www.intel.com/content/www/us/en/developer/articles/troubleshooting/c2686-when-using-msvc-as-host-compiler.html

dm-vodopyanov · 2021-11-17T18:13:09Z

@steffenlarsen, can you try exactly the same syntax of std::enable_if_t from https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/accessor_property_list.hpp?
It turned out this combination of std::enable_t syntax works for MSVC in accessor_property_list.
This patch made it work: #4240.
I hope it helps for property_list.

Signed-off-by: Steffen Larsen <[email protected]>

sycl/include/sycl/ext/oneapi/property_list/property_value.hpp

steffenlarsen · 2021-11-18T08:37:59Z

@steffenlarsen, can you try exactly the same syntax of std::enable_if_t from https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/accessor_property_list.hpp? It turned out this combination of std::enable_t syntax works for MSVC in accessor_property_list. This patch made it work: #4240. I hope it helps for property_list.

Thank you! That seems to have done the trick.

Signed-off-by: Steffen Larsen <[email protected]>

Runtime properties for the new property lists need to be seperately defined from the ones defined in SYCL 2020. These changes remove the support for SYCL 2020 properties in the compile-time property_list and adds the notion of runtime properties as a separate system. Signed-off-by: Steffen Larsen <[email protected]>

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen · 2022-01-21T12:21:39Z

Recent changes:

Moved the implementation into sycl::ext::oneapi::experimental.
Adjusted for updates in [SYCL][Doc] Update property_list specification to revision 2 #5338.

Signed-off-by: Steffen Larsen <[email protected]>

ghost · 2022-02-03T11:10:22Z

I'm not going to dive in details but from high-level perspective the patch looks good for me.

gmlueck · 2022-02-03T13:48:30Z

Could you please tell if the patch looks OK to you and if you are planning/not planning to review it?

Of the LEWG members, @rolandschulz was most involved in the implementation details of compile-time properties. I was not planning to review in detail, but @rolandschulz might want to.

bader · 2022-02-08T11:01:22Z

@rolandschulz, could you take a look, please?

bader · 2022-02-11T14:17:02Z

@rolandschulz, ping.

steffenlarsen · 2022-02-15T21:58:58Z

@rolandschulz - Could you please clarify if you plan on reviewing this? If not, @Pennycook would you mind having a look?

Pennycook · 2022-02-15T23:14:20Z

@rolandschulz - Could you please clarify if you plan on reviewing this? If not, @Pennycook would you mind having a look?

Like Greg, I wasn't planning to do an in-depth review here.

I did take a look when the PR was opened, but the template wizardry being employed is sufficiently advanced that I wouldn't feel comfortable reviewing for things like impact on compile-time, etc.

FWIW, what I understood looks good, and I appreciated the "How to add a new property" comments -- I think those will be very useful. 👍

cperkinsintel

Lime Gin Tastes Metallic!

cperkinsintel · 2022-02-23T23:22:47Z

sycl/include/sycl/ext/oneapi/properties/property.hpp

+//     `sycl::ext::oneapi::experimental::detail::PropKind` representing the new
+//     property. Increment
+//     `sycl::ext::oneapi::experimental::detail::PropKind::PropKindSize`
+//  2. Define property key class with `value_t` that must be `property_value`


can these new property keys be in other namespaces?

Sure. There is no requirement on the namespace of the properties. It would probably be preferable to have them in a namespace under sycl::ext::oneapi::experimental while it is still experimental, but since it is user-facing it should not be a requirement from the implementation.

sycl/include/sycl/ext/oneapi/properties/property.hpp

cperkinsintel · 2022-02-24T01:01:52Z

sycl/test/extensions/properties/mock_compile_time_properties.hpp

+namespace detail {
+template <> struct PropertyToKind<bar_key> {
+  static constexpr PropKind Kind =
+      static_cast<enum PropKind>(PropKind::PropKindSize + 0);


I'm guessing the awkward PropKindSize + X constructs here is because we aren't actually making modifications to property.hpp, correct?

Exactly. These properties are not "real" so we make their PropKind outside the usual range to avoid confusing them with actual properties.

cperkinsintel · 2022-02-24T01:06:59Z

sycl/include/sycl/ext/oneapi/properties/property.hpp

+// (4.)
+template <> struct is_property<bar_key> : std::true_type {};
+// Replace SYCL_OBJ with the SYCL object to support the property.
+template <> struct is_property_of<bar_key, SYCL_OBJ> : std::true_type {};


what if the SYCL_OBJ class itself requires template specialization? Use a template template? Proxy? Will we have order-of-evaluation issues?

is_property_of is already a thing in SYCL 2020. For example, if we specialize for buffer we have something long the lines of

template <typename T, int Dimensions, typename AllocatorT> struct is_property_key_of<bar_key, buffer<T, Dimensions, AllocatorT, void>> : std::true_type {};

I don't believe there should be any issues with that.

questions

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen · 2022-02-25T09:41:11Z

Failures were unrelated. They have been reported and testing has been restarted.

steffenlarsen · 2022-02-25T12:25:18Z

@romanovvlad & @psamolysov-intel & @cperkinsintel - There has been some minor corrections to the how-to. Would you mind having another look?

ghost

I didn't review deeply, but what I see in the latest 3 commits looks good for me. Thanks, @steffenlarsen!

steffenlarsen · 2022-02-28T11:03:31Z

@intel/llvm-gatekeepers - Remaining questions have been answered. This should be ready for merge.

bader · 2022-02-28T14:12:04Z

@steffenlarsen, post-commit failed with

/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:54:51: error: unused parameter 'LHS' [-Werror,-Wunused-parameter]
operator==(const property_value<PropertyT, A...> &LHS,
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:55:51: error: unused parameter 'RHS' [-Werror,-Wunused-parameter]
           const property_value<PropertyT, B...> &RHS) {
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:62:51: error: unused parameter 'LHS' [-Werror,-Wunused-parameter]
operator!=(const property_value<PropertyT, A...> &LHS,
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:63:51: error: unused parameter 'RHS' [-Werror,-Wunused-parameter]
           const property_value<PropertyT, B...> &RHS) {
                                                  ^
In file included from /home/runner/work/llvm/llvm/src/sycl/source/backend/level_zero.cpp:9:
In file included from /home/runner/work/llvm/llvm/src/sycl/include/CL/sycl.hpp:67:
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/properties.hpp:99:[42](https://github.com/intel/llvm/runs/5358967864?check_suite_focus=true#step:9:42): error: unused parameter 'PropertyValues' [-Werror,-Wunused-parameter]
  Extract(std::tuple<PropertyValueTs...> PropertyValues) {
                                         ^
5 errors generated.

Could you fix these issues, please?

steffenlarsen · 2022-02-28T14:47:30Z

@steffenlarsen, post-commit failed with

/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:54:51: error: unused parameter 'LHS' [-Werror,-Wunused-parameter]
operator==(const property_value<PropertyT, A...> &LHS,
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:55:51: error: unused parameter 'RHS' [-Werror,-Wunused-parameter]
           const property_value<PropertyT, B...> &RHS) {
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:62:51: error: unused parameter 'LHS' [-Werror,-Wunused-parameter]
operator!=(const property_value<PropertyT, A...> &LHS,
                                                  ^
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/property_value.hpp:63:51: error: unused parameter 'RHS' [-Werror,-Wunused-parameter]
           const property_value<PropertyT, B...> &RHS) {
                                                  ^
In file included from /home/runner/work/llvm/llvm/src/sycl/source/backend/level_zero.cpp:9:
In file included from /home/runner/work/llvm/llvm/src/sycl/include/CL/sycl.hpp:67:
/home/runner/work/llvm/llvm/build/include/sycl/ext/oneapi/properties/properties.hpp:99:[42](https://github.com/intel/llvm/runs/5358967864?check_suite_focus=true#step:9:42): error: unused parameter 'PropertyValues' [-Werror,-Wunused-parameter]
  Extract(std::tuple<PropertyValueTs...> PropertyValues) {
                                         ^
5 errors generated.

Could you fix these issues, please?

#5684 should fix it.

steffenlarsen requested review from rolandschulz, Pennycook, gmlueck, dm-vodopyanov and romanovvlad November 17, 2021 15:14

steffenlarsen requested a review from a team as a code owner November 17, 2021 15:14

steffenlarsen added 2 commits November 17, 2021 18:23

Fix formatting

861af21

Signed-off-by: Steffen Larsen <[email protected]>

Fix formatting but clang-format does not agree

27a9e26

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen added 2 commits November 17, 2021 21:32

Mark test XFAIL on CUDA

c61d569

Signed-off-by: Steffen Larsen <[email protected]>

Attempt to fix MSVC overload issue

1f283f9

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen changed the title ~~[SYCL] Adds compile-time property list~~ WIP: [SYCL] Adds compile-time property list Nov 17, 2021

steffenlarsen marked this pull request as draft November 17, 2021 21:15

cperkinsintel reviewed Nov 17, 2021

View reviewed changes

sycl/include/sycl/ext/oneapi/property_list/property_value.hpp Outdated Show resolved Hide resolved

steffenlarsen added 4 commits November 18, 2021 11:54

Propagate is_device_copyable on property_list

f565f5c

Signed-off-by: Steffen Larsen <[email protected]>

Propagate is_device_copyable onto const

9601c4c

Signed-off-by: Steffen Larsen <[email protected]>

Fix formatting

31d391e

Signed-off-by: Steffen Larsen <[email protected]>

AaronBallman self-requested a review December 2, 2021 19:55

steffenlarsen added 2 commits January 14, 2022 16:43

Move property_list to experimental

0f9bf8f

Signed-off-by: Steffen Larsen <[email protected]>

Adjust for renaming

43149a4

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen marked this pull request as ready for review January 21, 2022 12:23

steffenlarsen changed the title ~~WIP: [SYCL] Adds compile-time property list~~ [SYCL] Adds compile-time properties Jan 21, 2022

steffenlarsen added 3 commits January 21, 2022 16:12

XFAIL properties_device_copyable on HIP backend

335be0c

Signed-off-by: Steffen Larsen <[email protected]>

Fix XFAIL

5e6cbac

Signed-off-by: Steffen Larsen <[email protected]>

Fix XFAIL part 2

83a9875

Signed-off-by: Steffen Larsen <[email protected]>

ghost previously approved these changes Feb 3, 2022

View reviewed changes

cperkinsintel previously approved these changes Feb 23, 2022

View reviewed changes

cperkinsintel reviewed Feb 23, 2022

View reviewed changes

cperkinsintel reviewed Feb 24, 2022

View reviewed changes

sycl/include/sycl/ext/oneapi/properties/property.hpp Outdated Show resolved Hide resolved

cperkinsintel reviewed Feb 24, 2022

View reviewed changes

Fix reference to is_property in how-to

4608676

Signed-off-by: Steffen Larsen <[email protected]>

steffenlarsen dismissed stale reviews from ghost and romanovvlad via 4608676 February 24, 2022 11:46

steffenlarsen requested review from cperkinsintel, a user and romanovvlad February 24, 2022 11:46

steffenlarsen added 2 commits February 24, 2022 14:53

Fix formatting

de41a0f

Signed-off-by: Steffen Larsen <[email protected]>

Adhere to new formatting

d33bc77

Signed-off-by: Steffen Larsen <[email protected]>

ghost approved these changes Feb 28, 2022

View reviewed changes

romanovvlad approved these changes Feb 28, 2022

View reviewed changes

bader merged commit 87f60f6 into intel:sycl Feb 28, 2022

steffenlarsen deleted the steffen/compile_time_propperty_list branch December 6, 2023 11:38

[SYCL] Adds compile-time properties #4976

[SYCL] Adds compile-time properties #4976

Uh oh!

Conversation

steffenlarsen commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen commented Nov 17, 2021

Uh oh!

dm-vodopyanov commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

steffenlarsen commented Nov 18, 2021

Uh oh!

steffenlarsen commented Jan 21, 2022

Uh oh!

ghost commented Feb 3, 2022

Uh oh!

gmlueck commented Feb 3, 2022

Uh oh!

bader commented Feb 8, 2022

Uh oh!

bader commented Feb 11, 2022

Uh oh!

steffenlarsen commented Feb 15, 2022

Uh oh!

Pennycook commented Feb 15, 2022

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

cperkinsintel Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cperkinsintel Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

cperkinsintel Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Feb 25, 2022

Uh oh!

steffenlarsen commented Feb 25, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Feb 28, 2022

Uh oh!

bader commented Feb 28, 2022

Uh oh!

steffenlarsen commented Feb 28, 2022

Uh oh!

Uh oh!

steffenlarsen commented Nov 17, 2021 •

edited

Loading

dm-vodopyanov commented Nov 17, 2021 •

edited

Loading