wgsl: access mode is no longer an attribute #1735

dneto0 · 2021-05-17T21:55:40Z

Delete 'access' as an attribute.
Add "Memory Access Mode" section
Storage classes table gets a "Supported access modes" and "Default
access mode"
NOTE: This is the minimally flexible set of options; only 'storage'
has any flexibility.
Add "Access Modes Defaults" section, among "Memory View Types"
Each memory view has an access mode.
Access modes are inherited by most reference-handling operations.
LHS of assignment must be write or read-write
Load Rule applies only if the reference is read or read-write.
Storage texture types are parameterized by 'read' or 'write' access
mode.
Formal parameters that are pointers must match in access type with
the call site's pointer argument.
Fix the description of modf and frexp to properly parameterize
the pointer types.

dneto0 · 2021-05-17T21:57:04Z

Note: With this PR, the storage classes have minimum flexibility. Only the 'storage' storage class allows more than one access mode. (You can't create a read-only variable in function storage class, for example. )

github-actions · 2021-05-17T21:58:06Z

Previews, as seen when this build job started (3f71d6b):
WebGPU | IDL
WGSL
Explainer

alan-baker

I would merge both access modes into a single grammar.

wgsl/index.bs

kvark · 2021-05-18T14:29:46Z

wgsl/index.bs

-      // 'ptr<function,i32>' is the type of a pointer value that references storage
-      // for keeping an 'i32' value, using memory locations in the 'function' storage
-      // class.  Here 'i32' is the pointee type.
+      // 'ptr<function,i32,read_write>' is the type of a pointer value that references


are we forced to write read_write for all the pointers with this change?

Only for storage buffers. Line 1806 effectively says if there is only one valid mode, then that is the default and it must not be specified. So this comment is for exposition only as it is invalid WGSL.

With type inference for let-declarations, you never have to write this pointer type. (At least while pointer-to-storage is disallowed as in the baseline. And afterward you only need to write it for formal parameter declaration.)

This just mentions ptr<function,i32,read_write>, which confused me. Is this a valid type to write, given that the access should only be seen on storage and workgroup classes?

kvark · 2021-05-18T14:33:16Z

wgsl/index.bs

-textureLoad(t: [[access(read)]] texture_storage_2d<F>, coords: vec2<i32>) -> vec4<T>
-textureLoad(t: [[access(read)]] texture_storage_2d_array<F>, coords: vec2<i32>, array_index: i32) -> vec4<T>
-textureLoad(t: [[access(read)]] texture_storage_3d<F>, coords: vec3<i32>) -> vec4<T>
+textureLoad(t: texture_storage_1d<F,read>, coords: i32) -> vec4<T>


what's going to happen with read-write textures? are we going to provide overloads for read-write for all of the builtin functions on storage textures?

Similar to the proposal for coercing read_write pointers to read pointers, we could do the same thing with textures when read_write textures are added to WGSL. Or, yes, we could just provide overloads.

I would say we add overloads for read_write. We don't need to add coercion for this.

wgsl/index.bs

dneto0 · 2021-05-19T20:47:18Z

I've addressed actionable feedback. PTAL.

- Delete 'access' as an attribute. - Add "Memory Access Mode" section - Storage classes table gets a "Supported access modes" and "Default access mode" NOTE: This is the minimally flexible set of options; only 'storage' has any flexibility. - Add "Access Modes Defaults" section, among "Memory View Types" - Each memory view has an access mode. - Access modes are inherited by most reference-handling operations. - LHS of assignment must be write or read-write - Load Rule applies only if the reference is read or read-write. - Storage texture types are parameterized by 'read' or 'write' access mode. - Formal parameters that are pointers must match in access type with the call site's pointer argument. - Fix the description of modf and frexp to properly parameterize the pointer types. Fixes: gpuweb#1604

Mark 'read' as the default in the 'storage' row. The default-ness is already fully explained in its own section 'Memory Access Defaults'.

Instead of storage_textures_access and variable_access_mode, use a single access_mode and rely on external validation to catch the invalid combinations.

Move the grammar rule to the "Memory Access Mode" section.

dneto0 · 2021-05-21T20:24:29Z

Rebased and resolved conflicts. PTAL

kvark

If I have a pointer, do I need to specify the storage access with this PR?

always
only for storage and workgroup classes

kvark · 2021-05-26T14:49:59Z

wgsl/index.bs

-      // 'ptr<function,i32>' is the type of a pointer value that references storage
-      // for keeping an 'i32' value, using memory locations in the 'function' storage
-      // class.  Here 'i32' is the pointee type.
+      // 'ptr<function,i32,read_write>' is the type of a pointer value that references


This just mentions ptr<function,i32,read_write>, which confused me. Is this a valid type to write, given that the access should only be seen on storage and workgroup classes?

dneto0 · 2021-05-26T17:00:10Z

If I have a pointer, do I need to specify the storage access with this PR?

always

only for storage and workgroup classes

(Edit to fix the answer.)

Only for storage storage class. The comments in this PR show the fully elaborated type for analysis, but not for spelling it in the shader source. I'll call that out explicitly after defining the ref and ptr types.

dneto0 · 2021-05-27T13:30:41Z

Checked again with @kvark, acceptable to merge.

@dneto0

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@dneto0

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@dneto0

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

kdashg · 2021-06-01T07:52:57Z

WGSL meeting minutes 2021-05-25

(wgsl: access mode is no longer an attribute)
(Had one round of review, and addressed.)
(Can we land this?)
DM: When do we specify access quals for pointers. Spec has it for function. I will follow up offline.

Remove access control decorations. See gpuweb/gpuweb#1735 arrayLength() now takes a pointer to the array. See gpuweb/gpuweb#1619

Issue gpuweb#1202

dneto0 added the wgsl resolved Resolved - waiting for a change to the WGSL specification label May 17, 2021

dneto0 added this to the MVP milestone May 17, 2021

dneto0 requested review from alan-baker, kvark and litherum May 17, 2021 21:55

dneto0 added wgsl WebGPU Shading Language Issues wgsl resolved Resolved - waiting for a change to the WGSL specification and removed wgsl resolved Resolved - waiting for a change to the WGSL specification labels May 17, 2021

alan-baker requested changes May 18, 2021

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

kvark reviewed May 18, 2021

View reviewed changes

This was referenced May 19, 2021

Errant reference to break if in WGSL spec #1745

Closed

Add some text to describe function variables going out of scope #1748

Merged

dneto0 added 4 commits May 21, 2021 16:22

Remove 'default access mode' column

b04aca7

Mark 'read' as the default in the 'storage' row. The default-ness is already fully explained in its own section 'Memory Access Defaults'.

Combine grammar rules for access modes

1c68413

Instead of storage_textures_access and variable_access_mode, use a single access_mode and rely on external validation to catch the invalid combinations.

Move access_mode grammar rule

3e4dfcd

Move the grammar rule to the "Memory Access Mode" section.

dneto0 force-pushed the access-attribute branch from 08e6178 to 3e4dfcd Compare May 21, 2021 20:23

kvark approved these changes May 26, 2021

View reviewed changes

Emphasize difference between ref/ptr for analysis vs. in source text

fadf30b

dneto0 merged commit 7da3482 into gpuweb:main May 27, 2021

github-actions bot added a commit that referenced this pull request May 27, 2021

wgsl: access mode is no longer an attribute (#1735)

456df49

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request May 27, 2021

wgsl: access mode is no longer an attribute (#1735)

fdba11f

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request May 27, 2021

wgsl: access mode is no longer an attribute (#1735)

d79785b

SHA: 7da3482 Reason: push, by @dneto0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

ben-clayton added a commit to ben-clayton/webgpu-samples that referenced this pull request Jun 11, 2021

Updates for WGSL spec changes

8e6d6ab

Remove access control decorations. See gpuweb/gpuweb#1735 arrayLength() now takes a pointer to the array. See gpuweb/gpuweb#1619

ben-clayton mentioned this pull request Jun 11, 2021

Updates for WGSL spec changes webgpu/webgpu-samples#116

Merged

austinEng pushed a commit to webgpu/webgpu-samples that referenced this pull request Jun 11, 2021

Updates for WGSL spec changes

d5f67a9

Remove access control decorations. See gpuweb/gpuweb#1735 arrayLength() now takes a pointer to the array. See gpuweb/gpuweb#1619

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022

Implement cosh f32 tests (gpuweb#1735)

ed11c6b

Issue gpuweb#1202

wgsl: access mode is no longer an attribute #1735

wgsl: access mode is no longer an attribute #1735

Uh oh!

Conversation

dneto0 commented May 17, 2021

Uh oh!

dneto0 commented May 17, 2021

Uh oh!

github-actions bot commented May 17, 2021

Uh oh!

alan-baker left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dneto0 commented May 19, 2021

Uh oh!

dneto0 commented May 21, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dneto0 commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dneto0 commented May 27, 2021

Uh oh!

kdashg commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dneto0 commented May 26, 2021 •

edited

Loading