Remove the read-only texture storage bindings by kvark · Pull Request #1794 · gpuweb/gpuweb

kvark · 2021-06-02T15:00:43Z

Closes #1785
I think, it's another case of: if you have trouble naming something, then it's not a naming problem, it's an architecture problem. So in this case, the confusion is resolved by removing one of the ambiguous variants, and aligning the naming accordingly (SHADER_READ/SHADER_WRITE usages).

Preview | Diff

github-actions · 2021-06-02T15:05:46Z

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

github-actions · 2021-06-02T15:22:47Z

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

Kangz · 2021-06-02T15:32:47Z

spec/index.bs

-    const GPUFlagsConstant SAMPLED           = 0x04;
-    const GPUFlagsConstant STORAGE           = 0x08;
+    const GPUFlagsConstant SHADER_READ       = 0x04;
+    const GPUFlagsConstant SHADER_WRITE      = 0x08;


This means I can have a texture with only SHADER_WRITE that's bound to a var t : [[access(read)]] texture_storage_2d<r32float>?

RO storage textures are removed from WGSL as well, so you can't have that

Weird, so we'll have write, read_write in the future (with potentially coercions between them) but not read?

Would you need SHADER_READ | SHADER_WRITE for read_write, or just SHADER_WRITE? If the latter, maybe SHADER_READ/SHADER_READWRITE?

Never mind, I see you addressed this while I was writing the comment :)

@Kangz correct, yes. Slightly weird for people coming from GLSL/Vulkan (not for anybody else), but I think it's ok.

@kainino0x we talked about this on office hours earlier today, and concluded that STORAGE needs to be the name for the mutable usage, still, instead of SHADER_WRITE. So once read-write is added, it's still in STORAGE usage, and it's less confusing.

github-actions · 2021-06-02T15:34:47Z

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

Kangz · 2021-06-02T15:34:51Z

This PR is doing a bit too many things at the same time, we should discuss the removal of readonly-storage-texture separately (or before) the renaming of texture usages. It's not clear yet that we should remove that binding type.

kvark · 2021-06-02T15:45:35Z

Renaming the texture usages would be plain wrong if RO storage textures is still a thing. Keeping the old names would make little sense if the RO storage textures are gone.
I think this PR is fine as an input for discussing the topic. Once we figure out what needs to be done, we can rewrite it. No need to be picky right away.

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

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

Kangz · 2021-06-15T14:50:22Z

Following up on the discussion from the meeting yesterday.

There are two reasons why I think we shouldn't remove readonly storage texture:

It can use a different hardware path compared to regular sampled textures. This Twitter thread was most interesting: it explains that most texture caches are in-order to prioritize simplicity and reuse over latency. On the other hand storage textures are accessed via regular access to memory (like storage buffer accesses) that different characteristics (like less latency). SPIR-V's OpImageTexelPointer that gives a regular pointer for a texel of a storage texture.
Having readonly storage textures helps produce good barriers in compute passes. For example if a bunch of dispatches read data from storage texture A and spread it to writable resources distinct per dispatch. No barriers are needed between these dispatches because there is no data races: A is only read from and writable resources are written by a single dispatch each. Having readonly storage textures means that the level of access is encoded in the shader / in the layout: it is easier to get for the implementation and more explicit for the developer. This because even more important if/when we have more explicit barriers in compute passes.

If we decide to keep readonly storage texture, then we'll need to make sure they can be used as UAV in D3D12, so we'll need to change the "internal usages" to have different usages for readonly storage buffers / textures.

My suggestion is to set all texture storage usages (readwrite, readonly or writeonly) as "storage" and allowed to be used together, but I'd be ok fine with having readonly-storage be its own thing.

As a side-note: the "storage-write" internal usage doesn't seem useful at the moment, since its compatible usage list is fully included in the "storage" internal usage.

kvark · 2021-06-15T17:25:50Z

Want to stress on something I said during the last call. We aren't keeping the current r/o storage texture semantics. We are either changing it into the new semantic that maps to D3D12's UAV, or removing it. I believe a good investigation is in order before we can consider it. For example, loading from D3D12's UAVs requires the "UAV Typed Load" capability, which is the same as what we'll require for read-write storage textures. This means, effectively, that at least on D3D12 the set of texture formats supporting the newly suggested r/o storage semantics is the same as the future r/w storage. In this case, r/o storage brings no benefit: the only advantage versus r/w is being available on more formats, which isn't the case for D3D12.

With this in mind, I think we should proceed with removing the current semantics, and then follow-up with an investigation and a proposal, just like for any other new feature being discussed.

Kangz · 2021-07-08T19:12:35Z

I'm not sure this is worth debating this change more until we have read-write storage textures, especially since like you said @kvark there's limited D3D12 formats that can be written through UAVs. It's unfortunate that we'll remove them to add them back (at least that's my belief) but we have more important topics now.

kainino0x · 2021-07-12T19:23:50Z

Resolved: Remove current read-only storage textures (since we have to), and don't add the new ones yet, pending developer requests.

spec/index.bs

wgsl/index.bs

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

github-actions · 2021-07-13T22:28:39Z

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

@kvark

SHA: 52031c6 Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@kvark

SHA: 52031c6 Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@kvark

SHA: 52031c6 Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

1652: Switch read-only storage textures to be exclusive and behind a feature r=cwfitzgerald a=kvark **Connections** See gpuweb/gpuweb#1864 and gpuweb/gpuweb#1794 **Description** WebGPU API doesn't have RO storage textures right now. However, since `wgpu` already had a way to expose read-write storage textures, I figured we can just use it to gate read-only storage. **Testing** No examples currently exercise this path. Co-authored-by: Dzmitry Malyshau <[email protected]>

Kangz · 2021-07-15T09:32:29Z

Did we mean to rename SAMPLED with SHADER_READ? This wasn't discussed in the meeting as far as I can tell from the minutes and I think it is a very confusing change.

kvark · 2021-07-15T15:09:41Z

@Kangz we didn't want to keep SAMPLED for sure. If I understand your concern, once we have read-write or the new read-only storages, then people are going to be confused about SHADER_READ. What are the other options to name it?
cc @kainino0x

This was used in the old FP testing framework and is no longer used. It wasn't flagged as deadcode since the unittests referenced it, and I missed my TODO comments. Fixes gpuweb#1794

kvark added 3 commits June 2, 2021 11:29

Remove read-only texture storage bindings

c623064

Rename texture usages

6f9aea7

Remove RO storage textures from WGSL

8bcff94

kvark force-pushed the ro-storage branch from 950f54a to 8bcff94 Compare June 2, 2021 15:29

Kangz reviewed Jun 2, 2021

View reviewed changes

Rename SHADER_WRITE into STORAGE

aaf3a7e

kvark force-pushed the ro-storage branch from 3129cf3 to aaf3a7e Compare June 2, 2021 21:01

kvark mentioned this pull request Jun 21, 2021

Change RO texture usage to be restricted #1864

Closed

kvark added for webgpu editors meeting and removed for webgpu editors meeting labels Jul 12, 2021

kvark requested a review from kainino0x July 12, 2021 21:04

kainino0x approved these changes Jul 13, 2021

View reviewed changes

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

spec/index.bs Outdated Show resolved Hide resolved

wgsl/index.bs Outdated Show resolved Hide resolved

Apply suggestions from code review

51dc84f

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

kvark merged commit 52031c6 into gpuweb:main Jul 13, 2021

kvark deleted the ro-storage branch July 13, 2021 22:27

github-actions bot added a commit that referenced this pull request Jul 13, 2021

Remove the read-only texture storage bindings (#1794)

fac4fa4

SHA: 52031c6 Reason: push, by @kvark 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 Jul 13, 2021

Remove the read-only texture storage bindings (#1794)

12b587a

SHA: 52031c6 Reason: push, by @kvark 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 Jul 13, 2021

Remove the read-only texture storage bindings (#1794)

6dc8e97

SHA: 52031c6 Reason: push, by @kvark Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

kvark mentioned this pull request Jul 14, 2021

Switch read-only storage textures to be exclusive and behind a feature gfx-rs/wgpu#1652

Merged

Kangz mentioned this pull request Jul 19, 2021

Rename SHADER_READ back to SAMPLED #1963

Closed

This was referenced Jul 26, 2021

Rename texture usage flags to bindings #1989

Merged

Remove readable texture storage example #1993

Merged

kainino0x mentioned this pull request Aug 31, 2021

Add wgsl support for FromPixels tensorflow/tfjs#5552

Merged

teoxoy mentioned this pull request Dec 8, 2022

Restrict grammar of texture_and_sampler_types #3672

Closed

Conversation

kvark commented Jun 2, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 2, 2021

Uh oh!

github-actions bot commented Jun 2, 2021

Uh oh!

Kangz Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

Kangz Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

kainino0x Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

kainino0x Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 2, 2021

Uh oh!

Kangz commented Jun 2, 2021

Uh oh!

kvark commented Jun 2, 2021

Uh oh!

github-actions bot commented Jun 2, 2021

Uh oh!

Kangz commented Jun 15, 2021

Uh oh!

kvark commented Jun 15, 2021

Uh oh!

Kangz commented Jul 8, 2021

Uh oh!

kainino0x commented Jul 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 13, 2021

Uh oh!

Kangz commented Jul 15, 2021

Uh oh!

kvark commented Jul 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kvark commented Jun 2, 2021 •

edited by pr-preview bot

Loading