[wgsl] Elaborate storage classes section #1093

dneto0 · 2020-09-21T22:32:46Z

Also:

Define IO-shareable
Define read-only texture
Define write-only texture
Add more cross-links.
Restrict where runtime-sized arrays may be used
Store type of uniform and storage_buffer variables must be
block-decorated

kvark

Thank you for describing this!

kvark · 2020-09-22T03:29:29Z

wgsl/index.bs

@@ -395,21 +411,103 @@ The following types are <dfn dfn noexport>host-shareable</dfn>:
 * [[#array-types]] if the array type has a stride attribute and its element type is host-shareable
 * [[#struct-types]] if all its members are host-shareable

+The contents of uniform buffers and storage buffers must be of host-shareable type.
+
 TODO: *This is a stub*: Collectively, the *stride* and *offset* attributes are called *layout attributes*.

 ### Storage Classes TODO ### {#storage-class}


is this still TODO?

The description of layout is for a future MR. So the TODO is still warranted.

wgsl/index.bs

kvark · 2020-09-22T03:44:30Z

wgsl/index.bs

+      <td>[=Module scope=]
+      <td>[=Host-shareable=]
+      <td>See [[#memory-layout]].
+  <tr><td>image


Are we using "texture" and "image" interchangeably here? I understand that the term came from SPIR-V, where the relevant Vulkan API objects are images. In WGSL, it would be great to be consistent with WebGPU.

I'll change to "texture"

kvark · 2020-09-22T03:46:25Z

wgsl/index.bs

+      <td>Invocations in the same [=shader stage=]
+      <td>[=Module scope=]
+      <td>[=Host-shareable=]
+      <td>See [[#memory-layout]].


I find it confusing that we have 3 different uniform-ish storage classes. Was hoping to navigate to that [[#memory-layout] to understand more, but it's empty :)

Right, the memory-layout section is empty right now. That is yet another big change I would like to do separately.

There are 3 uniform-ish things:

storage_buffer. For OpenGL's SSBOs. Read-write buffers with explicit layout. Contents shareable with the host. Pretty flexible layout rules
- What's confusing is that originally Vulkan/SPIR-V had this in "Uniform" storage class with "BufferBlock" decorated struct. But now the style is "StorageBuffer" with Block-decorated struct

uniform. For OpenGL's UBOs. Read-only buffer with explicit layout. Strict and annoying array layouts. Limit max size. Contents shareable with host.

uniform_constant. For opaque things that happen to be handles, each with their own binding. The handles are immutable, but the underlying data might not be. "UniformConstant" is not my favourite name in the world but I suggest we bikeshed that separately.

This separation is rather confusing. First of all, it doesn't include storage images. I tried to check which storage class does glslang assign to them, and apparently I'm getting "UniformConstant" for a write-only storage image... It makes so little sense to me.

I guess the bigger question to ask is - why does WGSL need storage classes at all?

For one thing, you need to separate workgroup storage from private, for one thing.

Also, you have different rules for different storage classes. Layout rules differ.

Fundamentally, storage classes are a way of organizing the many rules about how memory treatedl

dj2 · 2020-09-22T12:40:19Z

wgsl/index.bs

+      <td>Same invocation only
+      <td>[=Module scope=]
+      <td>[=IO-shareable=]
+      <td>Contains values written by an upstream pipeline stage, or by the implementation.


Maybe

Input from an upstream pipeline stage, or the implementation

Trying to remove the word written as it's confusing when read with the output line below.

I didn't want it to read as circular. But your wording looks good.

dj2 · 2020-09-22T12:41:58Z

wgsl/index.bs

+      <td>Same invocation only
+      <td>[=Module scope=]
+      <td>[=IO-shareable=]
+      <td>Shader writes values for downstream pipeline processing.


Maybe

Output for a downstream pipeline stage. Zero initialized if required.

I'll take the first part.

The zero-initialized aspect is already described in "Variable and const" https://gpuweb.github.io/gpuweb/wgsl.html#variable-declaration

wgsl/index.bs

dneto0 · 2020-09-22T21:48:10Z

I think I've addressed all feedback.
I created a gist preview https://gistpreview.github.io/?2ab0184ed17ad6e4caea0a4f64af5794#storage-class

dj2 · 2020-09-23T13:28:49Z

wgsl/index.bs


 <pre class='def'>
 storage_class
  : INPUT
  | OUTPUT
+  | FUNCTION
+  | PRIVATE
  | UNIFORM
  | WORKGROUP
  | UNIFORM_CONSTANT
  | STORAGE_BUFFER
  | IMAGE


You renamed this to texture in the table below, should this also change to match?

It turns out we would only need this to express a pointer-to-image.
But that's only needed if we do image atomics.

However, we've learned that WGSL won't support image atomics. (see #728 (comment) ).

So the best thing is to remove "image" storage class altogether.

dj2 · 2020-09-23T20:32:17Z

wgsl/index.bs

+      <td>Read-only
+      <td>Invocations in the same shader stage
+      <td>[=Module scope=]
+      <td>Opaque representation of handle to a sampler or texture


What about if we dropped uniform_constant and we just used constant. Then, on the SPIR-V side if the variable is sampler or texture type we make a UniformConstant buffer?

I put an alternate proposal in: #1105 (comment)

dneto0 · 2020-10-02T17:01:43Z

I've removed the "image" (texture) storage class, and updated the gist: https://gistpreview.github.io/?2ab0184ed17ad6e4caea0a4f64af5794#storage-class

I think the main thing to focus on is what storage classes we need for textures, samplers, and buffers. #1105

dj2 · 2020-10-07T15:50:15Z

wgsl/index.bs

@@ -2943,26 +3111,22 @@ Issue: (dneto) Default rounding mode is an implementation choice.  Is that what
  <tr><td>`FOR`<td>for
  <tr><td>`FRAGMENT`<td>fragment
  <tr><td>`FUNCTION`<td>function
+  <tr><td>`HANDLE`<td>handle


Is this needed?

I moved it to reserved words.

If we use the full planned generality, then you can always get the base pointer of a variable and stuff it into a const. But we could make a special rule for that. I added a TODO for that in the PR.

dneto0 · 2020-10-07T19:46:34Z

I've rebased, and incorporated the feedback from this week's meeting.
I've updated the gist, and added more explanatory text in various places.

One choice that we can bikeshed about, is the name of the storage classes for UBO and SSBO.
I went with uniform and buffer because:

that matches GLSL and is short
when I tried "constant" I saw a lot of potential for confusion because "constant" is used all over the place, especially because const is a first class concept.

Note that the WebGPU spec has uniform-buffer and storage-bufferwhich are explicit but longer than I'd like.

dneto0 · 2020-10-07T19:47:18Z

Gist URL: https://gistpreview.github.io/?2ab0184ed17ad6e4caea0a4f64af5794#storage-class

PTAL

dneto0 · 2020-10-09T15:32:41Z

Rebased and resolved a conflict.

kvark · 2020-10-13T18:14:02Z

For consistency, let's use "Uniform" and "Storage" class names instead of "Uniform" and "Buffer".

litherum · 2020-10-13T18:14:25Z

Discussed 2020-10-13 https://docs.google.com/document/d/16vNf_rmJ566SoI-iOGGr-UqTGZ32SHKW2_qizoB4HxU/edit#heading=h.t2r4soa0e5hc

Also: - Define IO-shareable - Define read-only texture - Define write-only texture - Add more cross-links. - Restrict where runtime-sized arrays may be used - Store type of uniform and storage_buffer variables must be block-decorated Fixes gpuweb#1004

Highlight the difference between IO-shareable and host-shareable. Move a TODO down to the memory layout section. That whole section is still TODO.

Reword the introduction to the storage classes table, and on the "Variable scop" heading.

- Move restriction runtime arrays up to the section on array types. Add a TODO for tricky wording about not having intermediate values be of array type. - Add various cross-references - Move "block-decorated" restriction into the storage classes table so it's harder to miss. - Wording improvements for inputs and outputs

Rexture storage class would only be used if WGSL supports image atomics. But WebGPU won't have that because it's not available on all platform APIs. Also reorder the grammar list of storage classes to match the table.

- Rename: - storage_buffer -> buffer - uniform_constant -> handle - de-capitalize Private and Function storage class names in main text.

Also fill in details: Module-scope vars: - Describe restrictions based on storage class Function-scope vars - Describe restrictions on store type

The intent is to never have to write 'handle' in a WGSL program. Add TODO to prevent function paramater or const to be of pointer-to-handle type.

dneto0 · 2020-10-22T17:30:11Z

I've rebased against main, and updated the storage class name "buffer"->"storage".

I've updated the gist as well: https://gistpreview.github.io/?2ab0184ed17ad6e4caea0a4f64af5794#storage-class

dneto0 added the wgsl WebGPU Shading Language Issues label Sep 21, 2020

dneto0 added this to the MVP milestone Sep 21, 2020

dneto0 requested review from dj2 and litherum September 21, 2020 22:32

kvark reviewed Sep 22, 2020

View reviewed changes

dj2 reviewed Sep 22, 2020

View reviewed changes

dj2 reviewed Sep 23, 2020

View reviewed changes

kvark mentioned this pull request Sep 23, 2020

Which storage classes do we need? #1105

Closed

grorg added this to For Next Meeting in WGSL Sep 29, 2020

kvark approved these changes Oct 7, 2020

View reviewed changes

dj2 reviewed Oct 7, 2020

View reviewed changes

dneto0 force-pushed the storage-classes branch from 0f74704 to e2ef300 Compare October 7, 2020 19:39

dj2 approved these changes Oct 7, 2020

View reviewed changes

dneto0 force-pushed the storage-classes branch from e2ef300 to 107aba7 Compare October 9, 2020 15:32

dneto0 moved this from For Next Meeting to Open PRs (temp) in WGSL Oct 9, 2020

dneto0 mentioned this pull request Oct 9, 2020

wgsl: describe builtin variables and how to declare them #1142

Merged

dneto0 mentioned this pull request Oct 22, 2020

[wgsl] Define the interface of an entry point in a WGSL program #774

Closed

dneto0 added 5 commits October 22, 2020 13:20

Explain host-shareable better

d3a7461

Highlight the difference between IO-shareable and host-shareable. Move a TODO down to the memory layout section. That whole section is still TODO.

Rename 'image' storage class to 'texture'

17bef5b

Remove TODO on storage classes section

76cd927

Reword the introduction to the storage classes table, and on the "Variable scop" heading.

dneto0 added 16 commits October 22, 2020 13:20

Fix headings on storage class translation table

831d592

Fix cross-reference

e9888c9

Fix image -> texture

5dc7969

Remove texture storage class

a44e293

Rexture storage class would only be used if WGSL supports image atomics. But WebGPU won't have that because it's not available on all platform APIs. Also reorder the grammar list of storage classes to match the table.

Rename input->in, output->out

8d6868b

Rename storage classes

01d006d

- Rename: - storage_buffer -> buffer - uniform_constant -> handle - de-capitalize Private and Function storage class names in main text.

Indicate mapping of storage classes to WebGPU binding types

e5c0cf0

Describe when storage decoration is required, optional, and forbidden

66b34e6

Also fill in details: Module-scope vars: - Describe restrictions based on storage class Function-scope vars - Describe restrictions on store type

No expression has a type of runtime-sized array

77a3fe6

Finish describing host-shareable (but not the layout rules)

efa5940

Reserve 'handle'

67a5305

The intent is to never have to write 'handle' in a WGSL program. Add TODO to prevent function paramater or const to be of pointer-to-handle type.

Break up a long line

e6ce97f

Improve the module-scope variables examples

d0be81e

Add stub text for Resource Interface

f22cf4a

Rename storage class: 'buffer' -> 'storage'

413d2bf

Change BUFFER -> STORAGE in grammar

dfe9fc2

dneto0 force-pushed the storage-classes branch from 9af50a3 to dfe9fc2 Compare October 22, 2020 17:27

dj2 merged commit 7a3f41b into gpuweb:main Oct 26, 2020

WGSL automation moved this from Open PRs to Done Oct 26, 2020

kvark mentioned this pull request Oct 27, 2020

Clean up the storage classes gfx-rs/naga#245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wgsl] Elaborate storage classes section #1093

[wgsl] Elaborate storage classes section #1093

dneto0 commented Sep 21, 2020

kvark left a comment

kvark Sep 22, 2020

dneto0 Sep 22, 2020

kvark Sep 22, 2020

dneto0 Sep 22, 2020

kvark Sep 22, 2020

dneto0 Sep 22, 2020

kvark Sep 22, 2020

dneto0 Oct 22, 2020

dj2 Sep 22, 2020

dneto0 Sep 22, 2020

dj2 Sep 22, 2020

dneto0 Sep 22, 2020

dneto0 commented Sep 22, 2020

dj2 Sep 23, 2020

dneto0 Oct 2, 2020

dj2 Sep 23, 2020

dneto0 Oct 2, 2020

dneto0 commented Oct 2, 2020

dj2 Oct 7, 2020

dneto0 Oct 7, 2020

dneto0 commented Oct 7, 2020

dneto0 commented Oct 7, 2020

dneto0 commented Oct 9, 2020

kvark commented Oct 13, 2020

litherum commented Oct 13, 2020

dneto0 commented Oct 22, 2020

[wgsl] Elaborate storage classes section #1093

[wgsl] Elaborate storage classes section #1093

Conversation

dneto0 commented Sep 21, 2020

kvark 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

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

dneto0 commented Sep 22, 2020

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

dneto0 commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dneto0 commented Oct 7, 2020

dneto0 commented Oct 7, 2020

dneto0 commented Oct 9, 2020

kvark commented Oct 13, 2020

litherum commented Oct 13, 2020

dneto0 commented Oct 22, 2020