Updated spec constant proposal to use NTTP. #1

Naghasan · 2019-12-04T08:22:36Z

This patch extended the module proposal to add better support for spec constant.
It also introduce a different way to name spec constant values.
Spec constant are named by creating an object of type spec_id and can be manipulated using NTTP.

rolandschulz · 2019-12-11T17:11:49Z

proposals/sycl_modules.md

+From the SYCL compiler's perspective (C++ compiler) the value is understood to be a runtime value and
+for this reason it is not possible to use such value in a `constexpr`.
+
+### Retrieving a spec constant placeholder


What happens for a program such as:

spec_id<int> id; queue.submit([&](handler h) { spec_constant<int, id> c1 = h.set_spec_constant<int, id>(1); spec_constant<int, id> c2 = h.set_spec_constant<int, id>(2); h.single_task([=]{ int v = c1.get_spec_constant(); }); });

Is this a valid program? What's the value of v? Is it 1 if native_spec_constant() is false and 2 if native_spec_constant() is true?

Is this a valid program?

I think this should be banned, this look very odd and may induce unnecessary complexity on runtimes (whether there is native support or not). I'll clarify this.

Is it 1 if native_spec_constant() is false and 2 if native_spec_constant() is true?

In all cases the result of native_spec_constant() does not affect the behavior.

What do you mean with ban? UB or require compiler error? I think the first isn't reasonable and the 2nd isn't possible.

What do you mean with ban?

Runtime exception.

UB or require compiler error? I think the first isn't reasonable and the 2nd isn't possible.

i agree with you, I don't see why we would have an UB here and a compiler error is not always possible for this.

A runtime exception looks like a good approach to me too.
Note this behaviour is already stated on https://github.com/KhronosGroup/SYCL-Shared/pull/1/files#diff-dd15ce97b1565238f2c026fc84b30239R918 even with a more restrictive approach.

This patch extended the module proposal to add better support for spec constant. It also introduce a different way to name spec constant values. Spec constant are named by creating an object of type `spec_id` and can be manipulated using NTTP.

…ec_constant per spec_id and per command group scope

keryell

Great proposal!
A few nitpicks and some comments.
Please clarify the PR title.

proposals/sycl_modules.md

keryell · 2019-12-16T16:25:42Z

proposals/sycl_modules.md

-template <typename T, typename ID = T>
-class spec_constant {
+template <typename T>
+class spec_id {


Which is obviously 1.2.1 or 2020, right ?
The problem is that it is a niche concept, so it should not recycle some well known abbreviations...
What about just

class specialization_constant_id {

instead? Since it is just to declare some id, it should probably fit on 1 line anyway.

I reused the SPIR-V naming convention (OpSpecConstant) as in-fine this is to allow its representation in SYCL.

I'm fine to change spec to specialization. I don't really see a need, but you are in a better position to say if this makes things easier to understand.

I made the change in this commit, ad02463

I'm not convince we gain anything with this extra verbosity though.

We gain a clarification between specification and specialization confusion.
Since this is a niche use case, I do not think it will jeopardize the programmer productivity but will help the reader not familiar with SPIR-V details...

will help the reader not familiar with SPIR-V details

Maybe I should stop following SPIR-V then :)

If you feel it is better, we'll stick to it, no strong opinion on this

proposals/sycl_modules.md

keryell · 2019-12-16T17:23:44Z

proposals/sycl_modules.md

+From the SYCL compiler's perspective (C++ compiler) the value is understood to be a runtime value and
+for this reason it is not possible to use such value in a `constexpr`.
+
+### Retrieving a spec constant placeholder


A runtime exception looks like a good approach to me too.
Note this behaviour is already stated on https://github.com/KhronosGroup/SYCL-Shared/pull/1/files#diff-dd15ce97b1565238f2c026fc84b30239R918 even with a more restrictive approach.

keryell · 2019-12-16T17:24:30Z

proposals/sycl_modules.md

+ - Getting the placeholder for a previously set specialization constant value;
+   - If no specialization constant where ever set, then the *placeholder* object holds the default value of its `spec_id`.
+ - Setting a specialization constant value and retrieving the placeholder
+   - Performance notice: this function may cause the SYCL runtime to implicitly recompile the relevant SYCL module with the given value. This may have an important effect on performances and users should check their vendor recommendations on how to efficiently uses this functionality.


Just think FPGA... ;-) But on the other hand an implementation could cache all the previous instances too... So it really depends on the use case.

You can say you don't support the feature, or rely on a similar strategy as the host one.

I added this note it may not be obvious what is happening under the hood. I expect implementations to cache generated kernels, but if the user changes the value too often, there is nothing the runtime can really do and this will create a significant slowdown.

As this function is the most convenient one, I think it is important to mention it.

proposals/sycl_modules.md

keryell · 2020-01-08T19:12:30Z

proposals/sycl_modules.md

+v 0.10:
+
+* Rework of the specialization constant proposal
+  * Add `specialization _id` and `specialization _constant` definitions


Curious spaces before _

that's odd,

anyway, now fixed

keryell · 2020-01-08T19:24:48Z

proposals/sycl_modules.md

+  // Available if module_status::executable
+  // Retrieve the value of the specialization constant associated with this module.
+  // Call only valid from the host.
+  template<class T, specialization_id<T>& s>


After a second review I have just realized the & here, meaning that the id is actually the address of the specialization_id object...
This changes quite a lot in my understanding of the design...

keryell · 2020-01-08T19:31:25Z

proposals/sycl_modules.md

+
+  // Available if module_status::executable
+  //              Only from C++17
+  // Set the value of the specialization constant.


Suggested change

// Set the value of the specialization constant.

// Get the value of the specialization constant.

keryell · 2020-01-08T19:36:34Z

proposals/sycl_modules.md

+  // Call only valid from the host.
+  template<auto& s>
+  specialization_constant<typename std::remove_reference_t<decltype(s)>::type, s>
+    get_specialization_constant(handler&);


While waiting for concepts to be mandatory in SYCL, at least you should specificy in the comment that s needs to be an existing specialization identier or it will throw. Also that the specialization constant has been previously initialized.

keryell · 2020-01-08T19:37:03Z

proposals/sycl_modules.md

+
+  // Available if module_status::executable
+  // Retrieve the value of the specialization constant associated with this module.
+  // Call only valid from the host.


Also that the specialization constant has been previously initialized or it will throw.

keryell · 2020-01-08T20:57:56Z

proposals/sycl_modules.md

+`specialization_id` is used by the *SYCL implementation* to *reference* constant whose
+concrete value will only be known during the application execution.
+To create a new `specialization_id` in a module, the user creates a `specialization_id` object with an *automatic* or *static* storage duration in the namespace or class scope.
+The user then uses references to this object to bind the `specialization_constant` value to the *identifier*.


I do not think it binds the specialization_constant value to the identifier.
If I understand correctly it binds the specialization_constant value to the identifier and the command-group handler.
So a single specialization_id` object can be used to define different constant in different kernels.

keryell · 2020-01-08T21:10:52Z

proposals/sycl_modules.md

+To create a new `specialization_id` in a module, the user creates a `specialization_id` object with an *automatic* or *static* storage duration in the namespace or class scope.
+The user then uses references to this object to bind the `specialization_constant` value to the *identifier*.
+
+`specialization_id` objects must be forward declarable and cannot be moved or copied.


Now that I understand that you are interested in the address of the object and not the value of the object, I need to think about how it works with extern and multiple translation units.
The object could be moved or copied but at the end it is just another object with a different address, so it represents another specialization_id.
Also since you are interested by the address, why do you care about "objects must be forward declarable"?
This address mechanism is different from the type approach used to name kernels and global pipes in Intel extension. Is it really simpler?

After playing with this, it looks like the specialization_id objects needs static storage.
But by looking at the C++20 draft, I cannot see this restriction in 13.4.2 Template non-type arguments [temp.arg.nontype]

Some experiment in https://github.com/keryell/teaching_C-plus-plus/blob/master/NTTP_ref.cpp and https://github.com/keryell/teaching_C-plus-plus/blob/master/NTTP_ref_2.cpp

rolandschulz · 2020-01-14T20:37:23Z

proposals/sycl_modules.md

+  // This allow the user to setup a default value for the specialization_id instance.
+  // The initialization of T must be evaluated at compile time to be valid.
+  template<class... Args >
+  explicit constexpr specialization_id(Args&&...);


Why does this take a parameter pack rather than an argument of type T?

keryell · 2020-02-24T22:49:35Z

@hfinkel gave a presentation in Prague during ISO C++ SG7: P1609R3 "C++ Should Support Just-in-Time Compilation" http://wg21.link/p1609 which reminded me this discussion. cplusplus/papers#388

At least his paper adds a new C++ paradigm that might be similar to this concept of specialization constant, from a C++ perspective. It could give to specialization constant a kind of true C++ citizenship...
But from what I understand, what is possible to do at the SPIR-V level is quite limited compared to generic template instantiation envisioned by Hal Finkel.

This new proposal remove the need to use `spec_constant` object and uses a kernel handler instead to retrieve the values. Signed-off-by: Victor Lomuller <[email protected]>

AerialMantis · 2020-03-24T14:03:17Z

Whilst writing the wording for the proposal I have made some observations/alterations:

I propose that we move the member function native_specialization_constant to the device_image interface, as a module can contain multiple device_images and some may support native specialization constants and some may not. This change has not yet been made in the wording MR.
As we are now moving to C++17 for SYCL 2020 I have only introduced the post C++17 overloads of has/set/get_specialization_constant.
I have renamed the member alias of specialization_id type to value_type to align with the C++ convention.
I have renamed use_specialization_constant to contains_specialization_constants, as the former was very similar in name to the other member functions so suggested that the member function would instruct the module to use specialization constants.

Naghasan force-pushed the spec-constant branch from ff734d4 to 1873595 Compare December 5, 2019 14:38

rolandschulz reviewed Dec 11, 2019

View reviewed changes

Naghasan force-pushed the spec-constant branch from afc5b4f to 5c6d8ed Compare December 12, 2019 16:35

Naghasan added 4 commits December 12, 2019 16:42

Remove the use of experimental namespace

7daa699

Add a small example

fe00ff2

Restrict to one call to handler::set_spec_constant or handler::get_sp…

72d7513

…ec_constant per spec_id and per command group scope

Naghasan force-pushed the spec-constant branch from 5c6d8ed to 72d7513 Compare December 12, 2019 16:44

keryell suggested changes Dec 16, 2019

View reviewed changes

Add typo and some editorial fixes

b29123a

Naghasan force-pushed the spec-constant branch from 6d27d0c to b29123a Compare January 7, 2020 14:04

Naghasan added 2 commits January 7, 2020 14:58

Change spec_* to specialization_

ad02463

Use auto in the code example

6510a76

keryell reviewed Jan 8, 2020

View reviewed changes

rolandschulz reviewed Jan 14, 2020

View reviewed changes

Rewrite proposal using the suggested kernel_handler.

eec1a59

This new proposal remove the need to use `spec_constant` object and uses a kernel handler instead to retrieve the values. Signed-off-by: Victor Lomuller <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated spec constant proposal to use NTTP. #1

Updated spec constant proposal to use NTTP. #1

Naghasan commented Dec 4, 2019

rolandschulz Dec 11, 2019

Naghasan Dec 12, 2019

rolandschulz Dec 12, 2019

Naghasan Dec 12, 2019

keryell Dec 16, 2019

keryell left a comment

keryell Dec 16, 2019

Naghasan Jan 7, 2020

Naghasan Jan 7, 2020

keryell Jan 8, 2020

Naghasan Jan 9, 2020 •

edited

Loading

keryell Dec 16, 2019

keryell Dec 16, 2019

Naghasan Jan 7, 2020

keryell Jan 8, 2020

Naghasan Jan 9, 2020

keryell Jan 8, 2020

keryell Jan 8, 2020

Naghasan Jan 9, 2020

keryell Jan 8, 2020

keryell Jan 8, 2020

keryell Jan 8, 2020

keryell Jan 8, 2020

keryell Jan 8, 2020 •

edited

Loading

rolandschulz Jan 14, 2020

keryell commented Feb 24, 2020 •

edited

Loading

AerialMantis commented Mar 24, 2020

	// Set the value of the specialization constant.
	// Get the value of the specialization constant.

Updated spec constant proposal to use NTTP. #1

Are you sure you want to change the base?

Updated spec constant proposal to use NTTP. #1

Conversation

Naghasan commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naghasan Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keryell commented Feb 24, 2020 • edited Loading

AerialMantis commented Mar 24, 2020

Naghasan Jan 9, 2020 •

edited

Loading

keryell Jan 8, 2020 •

edited

Loading

keryell commented Feb 24, 2020 •

edited

Loading