[wgsl] Update storage textures to use access qualifiers

[dj2]

This CL updates the storage textures to use an access qualifier instead of the typename
to specify read/write.

For example, texture_storage_ro_1d<type> -> [[access(read)]] texture_storage_1d<type>.

Issue #1159

[dj2]

This builds on top of #1180

[kvark]

I still find it unfortunate that we have this entirely different syntax for buffers and textures. Can't we similarly have buffer_storage<read> type instead of struct for storage buffers to be consistent?

[dj2]

So instead of:

[[access(read)]] struct Foo {
...
};

You'd have:

buffer_storage<read> {
   ...
};

We'd also still have struct types, but that means struct types would never be used in the interface? So, offsets would only apply to buffer_storage? Would UBO buffers be declared in the same way with buffer_storage ?

[grorg]

Discussed at 2020-11-03 meeting.

[kvark]

Strawman time!

Straw-1: consistent decorations:

[block] struct Foo {..};
var foo<storage> : [access(read_write)] Foo;
var bar: [access(write_only)] texture_storage_1d<rgba32f>;

Straw-2: consistent type generics:

storage_buffer<ReadWrite> Foo { .. };
var foo: Foo;
var bar: texture_storage_1d<WriteOnly, rgba32f>;

[dneto0]

Back to first principles:

Textures:

• Textures are opaque: there is no way in WGSL to poke at the inside of a texture object.
• The type of a texture is only used to select which functions can be used with the texture.
  It makes sense to wrap up all the texture-method-selection properties in the texture type.
  They can't get in the way for any other purpose.

Buffers:

• Buffers exist so that we can write WGSL expressions that poke around at the insides:
  • We use primitive WGSL expressions to form references to a sub-part of a buffer:
    - structure member reference .foo
    - array, matrix, and vector element reference [i]
    - vector component reference .z
  • We use primitive WGSL expressions to load from and store to those sub-parts via those references.
• Types are the means of controlling which of those kinds of expressions are valid, and what they mean.

Discussion:

• I think it's a false consistency to require the 'read-only'-ness of an object be expressed in the
  same way when the way of talking about reading and writing via those objects is entirely different.
  (Methods vs. primitive expressions)
• A struct is a mathematical concept (a composite value with named components).
  But the read-only-ness is a global property of the whole variable, because variable refers to storage.
  You can read or write to storage.
• I think we want only one concept of 'struct', not 3 or 4.

Straw proposals

Straw-3.1: storage textures have access parameter embedded in the type

var bar: texture_storage_1d<read,rgba32f>;

Straw-3.2: buffer read/read-write is attached to the variable, as var param

[[block]] struct Foo { ... };
var<storage,read> FooParam : Foo;
var<storage,read_write> FooW0 : Foo;

# I'd be fine having ReadWrite being the default for 'storage' storage class.
var<storage> FooW1 : Foo;   

Straw-3.3: buffer read/read-write is attached to the variable, as regular attribute.

[[block]] struct Foo { ... };
[[access(read)]]       var<storage> FooParam : Foo;
[[access(read_write)]] var<storage> FooW0 : Foo;

# I'd be fine having ReadWrite being the default for 'storage' storage class.
                       var<storage> FooW1 : Foo;

My preference would be 3.1 and 3.2 together.

[grorg]

Discussed at the 2020-11-10 meeting.

[kvark]

@dneto0

I think it's a false consistency to require the 'read-only'-ness of an object be expressed in the
same way when the way of talking about reading and writing via those objects is entirely different.
(Methods vs. primitive expressions)

Method call is an expression, too! Buffer assignment is a statement. We can say that access qualifiers reduce the set of available expressions and statements involving these global variables. Seems pretty consistent to me.

A struct is a mathematical concept (a composite value with named components).
But the read-only-ness is a global property of the whole variable, because variable refers to storage.
You can read or write to storage.

Well, the same argument can be applied to [[block]]: why would it be needed on a struct if it's a purely mathematical concept? Clearly, our structs are more than that. They are already aware of the fact they are linked to GPU buffers, so they might as well know if these buffers can be modified.

I think we want only one concept of 'struct', not 3 or 4.

That's a fair wish. Having one struct would make users happy.

Straw proposals

How would you feel about straw-3.4?
(have just been discussing this with @dj2 in the other channel):

[[block]] struct Foo {...};
var<storage> FooParam : [[access(read)]]Foo;
var<storage> FooW0 : [[access(read_write)]] Foo;
var bar: [[access(write)]] texture_storage_1d<rgba32f>;

[dj2]

I've rebased this on top of #1183 as they both provide the access qualifiers. I've moved the annotation for the textures to be on the type declaration, which makes them look the same as storage buffers.

var<storage> a : [[access(read_write)]] Buffer;
var t : [[access(write)]] texture_storage_1d<rgba32f>;

[dneto0]

I can live with Straw-3.4

I asked questions on #1183:

• clarify that types with different qualifiers form different types. (I think we already treat it that way, e.g. with 'block' on struct.)
• whether the access qualifier is mandatory on storage buffer. (Or does it default to read_write for storage buffer)

[grorg]

Discussed at 2020-11-17 meeting.

[kvark]

On the call today, we concluded that both parties express their arguments for/against consistency of textures and buffers.

Checking the thread here, it appears that @dneto0 already expressed the position in great detail. We concluded that Straw 3.4 is compatible with the first principles as written, and also is consistent (so a good compromise). @jdashg does the argument miss anything?

Another point I made when chatting with @dj2 is the host side. From WebGPU host API, here is how it looks:

const bgl = device.createBindGroupLayout({
  entries: [
    {
      binding: 0,
      visibility: GPUShaderStage.VERTEX,
      type: "readonly-storage-buffer", // <-
      hasDynamicOffset: false,
    },
    {
      binding: 0,
      visibility: GPUShaderStage.FRAGMENT,
      type: "readonly-storage-texture", // <-
      viewDimension: "2d",
      storageTextureFormat: "rgba8unorm",
    },
  ],
});

The way I imagine this being specified is along these lines:

• if the binding type is "readonly-storage-buffer", the WGSL variable has to be decorated with [[access(read_only)]]
• if the binding type is "storage-buffer", the WGSL variable has to be decorated with [[access(read_write)]]
• if the binding type is "readonly-storage-texture", the WGSL variable has to be declared with [[access(read_only)]]
• if the binding type is "writeonly-storage-texture", the WGSL variable has to be declared with [[access(write_only)]]

[grorg]

Discussed at the 2020-12-08 meeting.

[kdashg]

(Oh no, I lost my draft comment!)
[read_only] for textures and buffers feels subtly but conceptually different to me, despite surface similarity. The core of it is that [read_only] on buffers behaves like [read_only] FooT => const FooT but [read_only] Texture<FormatT> behaves like Texture<const FormatT>. We could choose to define const Texture to behave as Texture like some standard libraries do, but this coupling of the access qualifier of the container with the access qualifier of its contents is often not what you want.

Now, we can make [read_only] Texture mean whatever we want, and yes it would look similar to how buffers look, but I want to be clear about this subtle difference in type design here.

[grorg]

Resolution: Accept the PR (but there was a lot of discussion and at least the title should change)

[dj2]

I've updated this PR and fixed up the title/comment. I think it should be good to go, but it was a .... bad ... merge. Please read it over and make sure I didn't merge anything incorrectly.

[github-actions]

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_e6e68a3

[kvark]

Looks like your rebase succeeded!

[github-actions]

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_c8097bc

[github-actions]

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_c8097bc

All resolved.