[SYCL] Implement device_global headers #5959

steffenlarsen · 2022-04-04T16:22:30Z

This commit adds the device_global experimental headers. Additionally it also adds the notion of meta-names and meta-values to compile-time properties, allowing properties to define names and values that can be used with __sycl_detail__::add_ir_attributes_* attributes.

This PR also enables device global processing for sycl-post-link, generating decorations for properties and the device global specific decorations.

(See sycl_ext_oneapi_device_global)

This commit adds the device_global experimental headers. Additionally it also adds the notion of meta-names and meta-values to compile-time properties, allowing properties to define names and values that can be used with __sycl_detail__::add_ir_attributes_* attributes. Signed-off-by: Steffen Larsen <steffen.larsen@intel.com>

steffenlarsen · 2022-04-04T16:28:13Z

/verify with intel/llvm-test-suite#971

…headers

steffenlarsen · 2022-05-27T14:29:41Z

/verify with intel/llvm-test-suite#971

aelovikov-intel · 2022-07-07T17:47:22Z

sycl/include/sycl/ext/oneapi/device_global/device_global.hpp

+
+// Type-trait for checking if a type defines the -> operator.
+template <typename T, typename = void>
+struct HasDerefOperator : std::false_type {};


I think this is misleading - operator* is deref. This one is either MemberAccess or Arrow.

Good point. It has been renamed.

aelovikov-intel · 2022-07-07T17:48:47Z

sycl/include/sycl/ext/oneapi/device_global/device_global.hpp

+// Class in the device_global inheritance chain. This class will define the
+// -> operator for the device_global if the underlying type T either has the


Suggested change

// Class in the device_global inheritance chain. This class will define the

// -> operator for the device_global if the underlying type T either has the

// Class in the device_global inheritance chain. This class will define the

// 'operator->' for the device_global if the underlying type T either has the

As per our offline discussion, the chain has been flattened so this comment is no more. I have done similar renaming in other places though.

aelovikov-intel · 2022-07-07T17:59:38Z

sycl/test/extensions/device_global/device_global_properties.cpp

@@ -0,0 +1,106 @@
+// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s


What are the warnings we're ignoring here?

None in particular. It is just there to avoid any rogue warnings from giving false negatives.

I'd feel more confident if we didn't do that, but maybe I'm missing something.

I don't mind removing it. So it has been removed.

sycl/test/extensions/device_global/device_global_properties.cpp

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

…headers

…_variable_decorations extension Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2022-07-08T12:43:27Z

/verify with intel/llvm-test-suite#971

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2022-07-08T13:55:02Z

/verify with intel/llvm-test-suite#971

mdtoguchi

OK for driver

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2022-07-08T15:27:45Z

/verify with intel/llvm-test-suite#971

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

aelovikov-intel · 2022-07-08T16:42:44Z

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

+// (7.)
+template <> struct PropertyMetaName<bar_key::value_t> {
+  static constexpr const char *value = "sycl-bar";
+};
+template <> struct PropertyMetaValue<bar_key::value_t> {
+  static constexpr int value = 5;
+};


What are these magic values (sycl-bar, 5)?

They are just example values to show how to do it, following the guide further up.

aelovikov-intel · 2022-07-08T16:44:39Z

sycl/test/extensions/device_global/device_global_properties.cpp

@@ -0,0 +1,106 @@
+// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s


I'd feel more confident if we didn't do that, but maybe I'm missing something.

aelovikov-intel · 2022-07-08T16:47:04Z

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

+// Forward declaration.
+template <typename T, typename PropertyListT> class device_global;


nit:

Suggested change

// Forward declaration.

template <typename T, typename PropertyListT> class device_global;

template <typename T, typename PropertyListT> class device_global;

C++ syntax is clear about what it is :P

Force of habit! Has been removed.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen · 2022-07-08T18:40:12Z

/verify with intel/llvm-test-suite#971

sycl/test/extensions/device_global/device_global_properties.cpp

…headers

steffenlarsen · 2022-07-25T10:16:12Z

/verify with intel/llvm-test-suite#971

aelovikov-intel · 2022-07-25T16:30:25Z

sycl/include/sycl/ext/oneapi/device_global/device_global.hpp

+template <typename T, typename PropertyListT = detail::empty_properties_t>
+class
+#ifdef __SYCL_DEVICE_ONLY__
+    // FIXME: Temporary work-around. Remove when fixed.
+    [[__sycl_detail__::global_variable_allowed, __sycl_detail__::device_global]]
+#endif
+    device_global {
+  // This should always fail when instantiating the unspecialized version.
+  static_assert(is_property_list<PropertyListT>::value,
+                "Property list is invalid.");
+};


I don't think we need extra specialization for this. IMO, just moving the static assert to the class below would work as good. In fact, it's already there on line 101.

This is the base template and the one below is the specialization. We need to do it this way to unpack the properties inside the property list. However, we still run the risk of a user using the base-template here, e.g. as device_global<int, double> which should fail with the "Property list is invalid."

In fact, in all cases where the base template is used, the static_assert will fail, as otherwise it would have gone for the specialization below, but we cannot make it static_assert(false, ...) as it will always fail to compile, so we must make it dependent.

Can we limit base case to declaration-only instead of definition? If that wouldn't be trivial, I'm fine with the current approach.

We could, but I think for the sake of error reporting, having definition with a static assert helps inform the user of what is wrong, for example if they do a device_global<int, double>, they'd be told specifically there's a problem with the property list rather than just being told that there is no definition for the class. From their perspective, the is_property_list<PropertyListT>::value check is still relevant, even though we know that for this definition it won't ever be true.

aelovikov-intel · 2022-07-25T16:40:34Z

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

+  static constexpr const char *name = "";
+  static constexpr std::nullptr_t value = nullptr;


Shouldn't it be empty here to cause compile time errors when used with unknown PropertyT ?

We don't expect all properties to have meta-information, as not all of them will be relevant to the compiler. For example, properties with runtime values are unlikely to have any relevant information at compile-time, so by default they will just be ignored, which is the behavior of having an empty meta-name.

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

aelovikov-intel · 2022-07-25T17:15:37Z

sycl/include/sycl/ext/oneapi/device_global/device_global.hpp

+template <typename T, typename PropertyListT = detail::empty_properties_t>
+class
+#ifdef __SYCL_DEVICE_ONLY__
+    // FIXME: Temporary work-around. Remove when fixed.
+    [[__sycl_detail__::global_variable_allowed, __sycl_detail__::device_global]]
+#endif
+    device_global {
+  // This should always fail when instantiating the unspecialized version.
+  static_assert(is_property_list<PropertyListT>::value,
+                "Property list is invalid.");
+};


Can we limit base case to declaration-only instead of definition? If that wouldn't be trivial, I'm fine with the current approach.

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

steffenlarsen · 2022-07-25T18:57:42Z

@mdtoguchi - Could you please re-approve? The driver changes should be the same as when you last approved.

steffenlarsen · 2022-07-26T07:45:49Z

No code change since last verification run. Merging this.

steffenlarsen requested a review from gmlueck April 4, 2022 16:22

steffenlarsen requested a review from a team as a code owner April 4, 2022 16:22

steffenlarsen requested a review from smaslov-intel April 4, 2022 16:22

This was referenced Apr 4, 2022

[SYCL] Add device_global on-device tests intel/llvm-test-suite#971

Merged

[SYCL][Doc] Host pipe design #5850

Merged

Merge remote-tracking branch 'intel/sycl' into steffen/device_global_…

e862c8b

…headers

aelovikov-intel reviewed Jul 7, 2022

View reviewed changes

steffenlarsen added 3 commits July 8, 2022 04:53

Respond to comments, throw exception, and flatten inheritance chain

d491b5a

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

Merge remote-tracking branch 'intel/sycl' into steffen/device_global_…

b90e21e

…headers

Add device-globals flag to sycl-post-link and enable SPV_INTEL_global…

44ae389

…_variable_decorations extension Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen requested a review from a team as a code owner July 8, 2022 12:41

Fix driver tests

7a202b1

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

mdtoguchi previously approved these changes Jul 8, 2022

View reviewed changes

Allow multiple device_global initializations

76c15a6

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

steffenlarsen dismissed mdtoguchi’s stale review via 76c15a6 July 8, 2022 15:24

aelovikov-intel reviewed Jul 8, 2022

View reviewed changes

Addressing more comments

a78998d

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>

keryell reviewed Jul 14, 2022

View reviewed changes

sycl/test/extensions/device_global/device_global_properties.cpp Outdated Show resolved Hide resolved

Merge remote-tracking branch 'intel/sycl' into steffen/device_global_…

63bb88b

…headers

aelovikov-intel reviewed Jul 25, 2022

View reviewed changes

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

ab3e61c

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

aelovikov-intel approved these changes Jul 25, 2022

View reviewed changes

mdtoguchi approved these changes Jul 25, 2022

View reviewed changes

steffenlarsen merged commit 6c66f28 into intel:sycl Jul 26, 2022

		// Class in the device_global inheritance chain. This class will define the
		// -> operator for the device_global if the underlying type T either has the

		@@ -0,0 +1,106 @@
		// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note,warning %s

		// Forward declaration.
		template <typename T, typename PropertyListT> class device_global;

		static constexpr const char *name = "";
		static constexpr std::nullptr_t value = nullptr;

[SYCL] Implement device_global headers #5959

[SYCL] Implement device_global headers #5959

Uh oh!

Conversation

steffenlarsen commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffenlarsen commented Apr 4, 2022

Uh oh!

steffenlarsen commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steffenlarsen commented Jul 8, 2022

Uh oh!

steffenlarsen commented Jul 8, 2022

Uh oh!

mdtoguchi left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 8, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 8, 2022

Uh oh!

Uh oh!

steffenlarsen commented Jul 25, 2022

Uh oh!

aelovikov-intel Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Jul 25, 2022

Uh oh!

steffenlarsen commented Jul 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

steffenlarsen commented Apr 4, 2022 •

edited

Loading

aelovikov-intel Jul 25, 2022 •

edited

Loading