Describe module constants, including pipeline-overrideable by dneto0 · Pull Request #886 · gpuweb/gpuweb

dneto0 · 2020-06-24T21:43:33Z

Fixes #572

Includes a strict "static use" rule. It allows for a very
simple implementation (no assumed smarts for dead code elimination),
and shifts the burden to the application to cleanup for interface matching.

This features corresponds roughly to:

SPIR-V specialization constants, except you can't use them
in all the same places. For example, in SPIR-V an array
size may be a specialization constant scalar, but no in WGSL.
MSL function constants, except:
- MSL allows vector values
- MSL has a facility for excluding an entry point interface
  parameter based on the value of a boolean function constant.

dneto0 · 2020-06-24T21:49:40Z

Notes:

I used 'constant_id' for the attribute name. I'm happy to bikeshed over the name. This is what GLSL uses. I think that's better than the longer "pipelin_override_index" or similar.
It's only for scalars, a limitation from SPIR-V.
The size of an array is already limited to be a literal value. So this doesn't magically expand functionality for array creation and sizing
Includes the static-use rule. In particular, you can't guard the presence/absence of an interface variable based on use in control flow guarded by a spec constant. This places the least burden on implementations to do dead code elimination. That's good for consistency (easy implementation), but shifts the burden to applications if they need interface cleanup. This is very much debatable, but any such debate needs to be mindful of how to limit the complexity of assured-dead-code-elim. (Arguably I could see user-space tooling for partial specialization. We've got that for SPIR-V.)

--
edited to fix the last bullet's english.

dj2

How will these work when we translate to HLSL?

dj2 · 2020-06-24T22:02:36Z

wgsl/index.bs

+
+A *module constant* declares a name for a value, outside of all function declarations.
+The name is available for use after the end of the declaration,
+until the end of the WGSL program.


s/WGSL/[SHORTNAME]

dj2 · 2020-06-24T22:04:06Z

wgsl/index.bs

+The name is available for use after the end of the declaration,
+until the end of the WGSL program.
+
+When the declaration has no attributes, the value is the expression in the declaration.


Should we note that the initializer is required?

Well, the grammar requires there to be an initializer expression. So I don't think we need to say extra here.

dj2 · 2020-06-24T22:35:43Z

wgsl/index.bs

+    to a value of the constant's type.
+    If the mapping does not have an entry for the pipeline constant ID, then the expression
+    in the declaration is used as the value.
+  * Pipeline constant IDs must be unique within the WGSL program: Two module constants


nit: s/WGSL/[SHORTNAME]

dj2 · 2020-06-24T22:36:04Z

wgsl/index.bs

+  * The type must one of the [[#scalar-types]].
+  * The attribute's literal operand is known as the *pipeline constant ID*,
+    and must be a non-negative integer value representable in 32 bits.
+  * The application can specify its own value for the name at pipeline-creation time.


What happens if the application provides a value for an ID which doesn't exist in the shader?

I would say that's an application error. I would lean toward the requiring the pipeline creation API to fail.
The downside to that is it may cause more bookkeeping in the API.

Added this as an issue

kvark · 2020-06-25T18:55:49Z

wgsl/index.bs

+  : CONSTANT_ID INT_LITERAL
+</pre>
+
+Issue(dneto): The WebGPU pipeline creation API must specify how API-supplied values are mapped to


We have the concept of host-accessible types in the spec, right? Can we just say the type of a constant has to be a host-accessible scalar?

Hmm. That removes the ability to set booleans. It's just as easy to say that when setting a bool constant, supply a 32-bit integer value, with 0 mapping to false, and true otherwise. It depends on the shape of the API whether we even need that detail or not.

grorg · 2020-07-07T18:15:19Z

Discussed at the 2020-07-07 meeting.

litherum · 2020-07-07T20:18:10Z

wgsl/index.bs

+
+<pre class='def'>
+global_constant_decl
+  : global_const_decoration_list? CONST variable_ident_decl EQUAL const_expr


It should be possible for an author to omit the EQUAL const_expr

That changes the grammar for a 'const' declaration. The rule would be to require an initializer on the const if and only if not having the attribute.

For me that elevates the status of the attribute quite a bit. I'm tempted to use new keyword other than 'const', e.g. 'spec_const'. I have to think that over.

I went with making the expression optional. It didn't make things too complex.

dneto0 · 2020-07-07T20:37:38Z

(rebased against latest main)

dneto0 · 2020-07-07T21:07:48Z

(removed an accidental commit meant for another PR)

dneto0 · 2020-07-07T22:09:37Z

wgsl/index.bs

+    The pipeline creation API accepts a mapping from the pipeline constant ID
+    to a value of the constant's type.
+    If the mapping has an entry for the ID, the value in the mapping is used.
+    Otherwise, the initializer expression must be present, and its value is used.


This was the simplest way to say it.
But the implementation detects the error at pipeline creation time, not at shader module creation time. It's pipeline creation that fails, no earlier.

litherum · 2020-07-14T17:45:34Z

What about constants where the value isn't supplied by the WGSL source, and also isn't supplied by the API? Do they get filled by 0s? Or compiler error?

grorg · 2020-07-14T18:45:47Z

Discussed at the 2020-07-14 meeting.

Resolution was to accept the PR, but open an issue changing the numbers into strings.

dneto0 · 2020-07-14T18:49:57Z

What about constants where the value isn't supplied by the WGSL source, and also isn't supplied by the API? Do they get filled by 0s? Or compiler error?

From the PR:

Otherwise, the initializer expression must be present, and its value is used.

So that case should result in failure of pipeline creation.

Fixes gpuweb#572 Includes a strict "static use" rule. It allows for a very simple implementation (no assumed smarts for dead code elimination), and shifts the burden to the application to cleanup for interface matching. This features corresponds roughly to: - SPIR-V specialization constants, except you can't use them in all the same places. For example, in SPIR-V an array size may be a specialization constant scalar, but no in WGSL. - MSL function constants, except: - MSL allows vector values - MSL has a facility for excluding an entry point interface parameter based on the value of a boolean function constant.

dneto0 · 2020-07-14T21:21:13Z

Rebased to avoid the merge conflict.

dneto0 requested a review from litherum June 24, 2020 21:43

dneto0 requested review from dj2 and kvark June 24, 2020 21:54

dneto0 added the wgsl WebGPU Shading Language Issues label Jun 24, 2020

dneto0 added this to the MVP milestone Jun 24, 2020

dj2 reviewed Jun 24, 2020

View reviewed changes

kvark reviewed Jun 25, 2020

View reviewed changes

litherum reviewed Jul 7, 2020

View reviewed changes

dneto0 force-pushed the wgsl-spec-constant branch from 9b01e81 to ef525bc Compare July 7, 2020 20:37

dneto0 force-pushed the wgsl-spec-constant branch 2 times, most recently from 3b59c33 to ef525bc Compare July 7, 2020 21:07

dneto0 commented Jul 7, 2020

View reviewed changes

dneto0 mentioned this pull request Jul 8, 2020

SPIR-V Specialization Constants and Pointers KhronosGroup/OpenCL-Docs#344

Open

dneto0 added 4 commits July 14, 2020 17:19

Fix typo

41e14dd

Use SHORTNAME, note issue for populating an ID not in the shader

96b93f2

Initializers are optional for overridable constants

766ec76

dneto0 force-pushed the wgsl-spec-constant branch from 1e5c29a to 766ec76 Compare July 14, 2020 21:20

dneto0 requested review from dj2 and litherum July 14, 2020 21:21

dj2 approved these changes Jul 15, 2020

View reviewed changes

dj2 merged commit 8ab3f22 into gpuweb:main Jul 15, 2020

cwfitzgerald mentioned this pull request Aug 20, 2020

How would I pass a specialization constant? gfx-rs/wgpu-rs#521

Closed

kvark mentioned this pull request Aug 3, 2021

Pipeline overrides gfx-rs/wgpu#1762

Closed

Conversation

dneto0 commented Jun 24, 2020

Uh oh!

dneto0 commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dj2 left a comment

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!

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!

grorg commented Jul 7, 2020

Uh oh!

litherum Jul 7, 2020 • 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!

dneto0 commented Jul 7, 2020

Uh oh!

dneto0 commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litherum commented Jul 14, 2020

Uh oh!

grorg commented Jul 14, 2020

Uh oh!

dneto0 commented Jul 14, 2020

Uh oh!

dneto0 commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dneto0 commented Jun 24, 2020 •

edited

Loading

litherum Jul 7, 2020 •

edited

Loading