Add RO and RW storage textures by alan-baker · Pull Request #4326 · gpuweb/gpuweb

alan-baker · 2023-10-12T19:51:11Z

WGSL
- allow read and read_write access modes for storage textures
- allow ro and rw storage textures with textureLoad
- allow rw storage textures with textureStore
- add textureBarrier
WebGPU
- add new enums for read-only and read-write storage textures

Plan is to add API-side (not all the formats) to this PR too. Currently just the WGSL changes.

wgsl/index.bs

github-actions · 2023-10-12T19:56:11Z

Previews, as seen when this build job started (abbec21):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

spec/index.bs

alan-baker · 2023-10-12T20:49:15Z

CC @teoxoy

toji

API-side LGTM now, thanks!

One think I'd like to confirm, though: Does every texture format which supports STORAGE_BINDING usage support all three access modes? I feel like the answer should obviously be "yes" but I think we all know that GPUs do not always do the obvious thing. 😁

kainino0x

API spec LGTM, though good question from @toji

spec/index.bs

jrprice

As discussed during prototyping, this needs an entry for textureLoad() in the "Uniformity Rules for Function Calls" section to state that loads from a read-write storage texture may be non-uniform.

teoxoy

I think we should update this section.

One think I'd like to confirm, though: Does every texture format which supports STORAGE_BINDING usage support all three access modes? I feel like the answer should obviously be "yes" but I think we all know that GPUs do not always do the obvious thing. 😁

No, we need to add another column for read-only & read-write access. With the only formats supporting those accesses (in the core spec) being:

r32float
r32uint
r32sint

based on #3838 (comment) & #3838 (comment)

The other formats listed in the issue can be added later and be conditional on a new feature.

wgsl/index.bs

teoxoy · 2023-10-13T12:22:56Z

@alan-baker thanks for starting the PR!

I agreed on the last API call to add 2 proposals (one for adding the functionality to core and one for the rw-storage-texture-tier-1 feature). @Kangz since this PR adds the functionality to core already, would you prefer the rw-storage-texture-tier-1 feature be a PR or still proposal?

alan-baker · 2023-10-13T15:22:15Z

As discussed during prototyping, this needs an entry for textureLoad() in the "Uniformity Rules for Function Calls" section to state that loads from a read-write storage texture may be non-uniform.

Done, PTAL

alan-baker · 2023-10-13T15:35:11Z

@teoxoy I've made the changes to resource usage and texture compatibility, PTAL.

teoxoy

LGTM

toji

Thanks for the table update!

The big thing that's missing now is an update to the createBindGroupLayout() validation. I can't comment on the line directly in this PR, but at line 5991 where we say:

|entry|.{{GPUBindGroupLayoutEntry/storageTexture}}.{{GPUStorageTextureBindingLayout/format}} must be a format which can support storage usage.

It should be updated to something like:

|entry|.{{GPUBindGroupLayoutEntry/storageTexture}}.{{GPUStorageTextureBindingLayout/format}} is a format which supports the given |entry|.{{GPUBindGroupLayoutEntry/storageTexture}}.{{GPUStorageTextureBindingLayout/access}} according to the [Plain Color Formats](#plain-color-formats) table.

spec/index.bs

toji

Thanks! LGTM now!

spec/index.bs

Kangz · 2023-10-15T13:00:39Z

spec/index.bs

+                        : `read`
+                        :: Set |textureLayout|.{{GPUStorageTextureBindingLayout/access}} to {{GPUStorageTextureAccess/"read-only"}}
+                        : `write`
+                        :: Set |textureLayout|.{{GPUStorageTextureBindingLayout/access}} to {{GPUStorageTextureAccess/"write-only"}}


Could we have text to automatically upgrade write-only to read-write if needed (even if it is not usable in the API yet?)

Do you mean have the API "read-write" be compatible with WGSL "write" and use that if the format works?

Yes for WGSL writeonly being compatible with read-write in the layout. But also in the algorithm to default the GPUPipelineLayout of a pipeline we have a step to merge bindings between shader stages:

So here we'd have something like:

If resource is a storageTexture, previousEntry has access write-only and entry has access read-write, then set previousEntry's access to read-write.

When rw-storage-texture-tier-1 is introduced, you could merge bindings on some devices, but not others. Is that desirable?

Yes, that seems ok, because the optional features are opt-in.

I'm ok with doing this. Do we need larger group agreement first?

We can put it as tacit resolution.

PTAL at those changes.

Co-authored-by: Kai Ninomiya <[email protected]>

* Make the result of textureLoad from a read-write storage texture non-uniform

* Update texture format compatibility to only allow r32uint, r32sint, and r32float for readable storage textures

Co-authored-by: Brandon Jones <[email protected]>

* Allow WO storage textures to be compatible of with RW bindings if the formats are compatible * Allow entries to be merged according to that compatibility

Co-authored-by: Corentin Wallez <[email protected]>

Co-authored-by: Kai Ninomiya <[email protected]>

Pull gpuweb#4326 introduced temporary anchors for "read-only" and "read-write" storage texture access. This commit removes them since the real anchors are landed in the API spec.

Pull #4326 introduced temporary anchors for "read-only" and "read-write" storage texture access. This commit removes them since the real anchors are landed in the API spec.

…b/gpuweb#4326

alan-baker added this to the Milestone 1 milestone Oct 12, 2023

alan-baker requested review from dneto0, kainino0x, mehmetoguzderin and toji October 12, 2023 19:51

alan-baker commented Oct 12, 2023

View reviewed changes

wgsl/index.bs Outdated Show resolved Hide resolved

alan-baker force-pushed the read-storage-texture branch from 5de46c4 to 446a7ae Compare October 12, 2023 20:02

toji requested changes Oct 12, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

alan-baker marked this pull request as ready for review October 12, 2023 20:48

alan-baker requested a review from toji October 12, 2023 20:49

toji approved these changes Oct 12, 2023

View reviewed changes

kainino0x approved these changes Oct 12, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

mehmetoguzderin approved these changes Oct 13, 2023

View reviewed changes

jrprice requested changes Oct 13, 2023

View reviewed changes

teoxoy reviewed Oct 13, 2023

View reviewed changes

wgsl/index.bs Show resolved Hide resolved

alan-baker requested a review from jrprice October 13, 2023 15:22

alan-baker requested review from kainino0x and toji October 13, 2023 15:35

teoxoy approved these changes Oct 13, 2023

View reviewed changes

toji requested changes Oct 13, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

alan-baker requested a review from toji October 13, 2023 19:42

toji approved these changes Oct 13, 2023

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

kainino0x approved these changes Oct 13, 2023

View reviewed changes

Kangz reviewed Oct 15, 2023

View reviewed changes

alan-baker added the tacit resolution queue Editors have agreed and intend to land if no feedback is given label Oct 16, 2023

kainino0x and others added 15 commits October 20, 2023 11:03

Use anchor database to define new cross-refs

dce4d82

Update spec/index.bs

8934cc6

Co-authored-by: Kai Ninomiya <[email protected]>

Changes for review

413116b

* Make the result of textureLoad from a read-write storage texture non-uniform

Changes for review

666cda5

* Update texture format compatibility to only allow r32uint, r32sint, and r32float for readable storage textures

fix table

1193b5b

Update spec/index.bs

0fe83c6

Co-authored-by: Brandon Jones <[email protected]>

Update spec/index.bs

65410bd

Co-authored-by: Brandon Jones <[email protected]>

fix createBindGroupLayout validation

4ffd5c6

Update spec/index.bs

3b6500b

Co-authored-by: Brandon Jones <[email protected]>

Make WGSL write-only storage textures compatible with RW in the API

5d625e8

* Allow WO storage textures to be compatible of with RW bindings if the formats are compatible * Allow entries to be merged according to that compatibility

Update spec/index.bs

b491d28

Co-authored-by: Corentin Wallez <[email protected]>

Update spec/index.bs

584bb3d

Co-authored-by: Corentin Wallez <[email protected]>

Update spec/index.bs

c4d09da

Co-authored-by: Kai Ninomiya <[email protected]>

Remove separation between read and write storage formats

1557df7

put extra column back for read-write only

abbec21

alan-baker force-pushed the read-storage-texture branch from 4361ecf to abbec21 Compare October 20, 2023 15:05

alan-baker merged commit 7d903a0 into gpuweb:main Oct 20, 2023

alan-baker deleted the read-storage-texture branch October 20, 2023 15:13

kainino0x added needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet and removed tacit resolution queue Editors have agreed and intend to land if no feedback is given labels Oct 20, 2023

dneto0 mentioned this pull request Oct 20, 2023

support read-only, read-write storage textures gpuweb/cts#3078

Closed

teoxoy mentioned this pull request Oct 23, 2023

[wgsl-in] Fix expected min arg count of textureLoad gfx-rs/naga#2584

Merged

alan-baker mentioned this pull request Oct 30, 2023

Remove unnecessary anchrors for access mode #4354

Merged

This was referenced Nov 13, 2023

The bgra8unorm-storage feature should most likely only enable write-only access #4377

Closed

Align with the spec on read-only and read-write storage textures gfx-rs/wgpu#4704

Closed

rajveermalviya mentioned this pull request Nov 26, 2023

WGPUStorageTextureAccess missing access modes gfx-rs/wgpu-native#324

Closed

juj added a commit to juj/wasm_webgpu that referenced this pull request Mar 7, 2024

IDL: Add support for read-only and read-write storage textures. gpuwe…

fd25288

…b/gpuweb#4326

caugner mentioned this pull request Nov 21, 2024

Chrome 124 supports read-write and read-only storageTexture.access + Chrome Android 121 supports WGSLLanguageFeatures mdn/browser-compat-data#25137

Merged

Conversation

alan-baker commented Oct 12, 2023

Uh oh!

Uh oh!

github-actions bot commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alan-baker commented Oct 12, 2023

Uh oh!

toji left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrprice left a comment

Choose a reason for hiding this comment

Uh oh!

teoxoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teoxoy commented Oct 13, 2023

Uh oh!

alan-baker commented Oct 13, 2023

Uh oh!

alan-baker commented Oct 13, 2023

Uh oh!

teoxoy left a comment

Choose a reason for hiding this comment

Uh oh!

toji left a comment • edited by kainino0x Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toji left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

github-actions bot commented Oct 12, 2023 •

edited

Loading

toji left a comment •

edited by kainino0x

Loading