Refactor GPUBindGroupLayout to isolate binding type definitions #1223

toji · 2020-11-14T01:06:07Z

This is currently a WIP because this change touches a very large number of references in the spec and I haven't yet updated them all. Wanted to get this pushed to the server for the weekend, though, and figured you might be interested in taking a peek. Of special interest is the updated binding type table and the bind group validation logic.

One thing to note early on: The usage of the term "kind" has felt rather awkward as I've been writing out some of the spec prose here. I don't have a suggested alternative, just point out that I've found it hard to write about naturally.

Preview | Diff

kainino0x

I only skimmed this so far, just a few IDL nits. But overall I think the API structure is great.

re: kind. We could just use type, since we are removing the other field called type. Semantically it would specify which "type" among the "types" allowed by the GPU*LayoutEntry type. (Maybe we need a good word for "(GPUBufferLayoutEntry|GPUSamplerLayoutEntry|GPUTextureLayoutEntry|GPUStorageTextureLayoutEntry)" though.)

kainino0x · 2020-11-14T02:22:55Z

spec/index.bs


-    // Used for uniform buffer and storage buffer bindings. Must be undefined for other binding types.
-    GPUSize64 minBufferBindingSize;
+    // Used for uniform buffer and storage buffer bindings. Must be null for other binding types.


Fields are always optional unless they're required (or have a default, = {}). We don't need to make these nullable since the fields can be unset or undefined. (e.g. { binding: 0, visibility: 0, buffer: {} } would be valid).

kainino0x · 2020-11-14T02:25:05Z

spec/index.bs

+    GPUStorageTextureAccess kind;
+    GPUTextureFormat format;


These two should be required

kainino0x · 2020-11-14T02:29:55Z

spec/index.bs


-    // Used for uniform buffer and storage buffer bindings. Must be undefined for other binding types.
-    GPUSize64 minBufferBindingSize;
+    // Used for uniform buffer and storage buffer bindings. Must be null for other binding types.


We should take this opportunity to move this stuff from comments to real prose. The reason it was comments was because this dictionary was so difficult to understand in the first place (understanding which fields were used for which binding types). Now that we're solving that problem, we can simply say just below here, "Exactly one of .buffer, .sampler, .texture, or .storageTexture must be specified. Which is specified, and value of that sub-entry, determines what type of resource can be bound. (Note: a value of {} is valid if the type has no required members.)`

Kangz · 2020-11-16T15:05:40Z

I was surprised to see GPUStorageTextureLayoutEntry split from GPUTextureLayoutEntry. Maybe they could stay the same?

Otherwise the "kind" naming could be replaced by "type" so that would be GPUBufferBindingType GPUSamplerBindingType and GPUTextureBindingType. Kind vs. type was useful in prior iterations of the rework but don't make sense now that things are in sub-descriptors. WDYT?

kainino0x · 2020-11-16T22:16:54Z

I suggested that they be separate. There's only some overlap between the options for sampled vs storage textures as they're quite different semantically. They both have viewDimension, but samplingResultType and format are unique to one or the other. Since sampled textures are much more common than storage textures, I think it's worth keeping the additional field and "kind" values separate.

kainino0x · 2020-11-16T23:05:14Z

Editors meeting: Agreed on new wording (samplers are "filtering" or "non-filtering", textures are "filterable" or "non-filterable"). Texture bindings are either "texture" (not "sampled texture" as multisampled textures can't be sampled) or "storage texture". Not quite decided on how multisampling works yet. Filing an issue about that.

kainino0x · 2020-11-16T23:05:40Z

And we also decided to keep GPUStorageTextureLayoutEntry split from GPUTextureLayoutEntry.

toji · 2020-11-17T05:31:09Z

Updated to address the initial feedback, use the new naming that @kainino0x mentioned, and fix and remaining broken references. Verbiage is generally better now but still a bit inconsistent in places. Let me know if anything sounds odd or unwieldy to you!

Kangz · 2020-11-17T14:05:44Z

I'm still a bit lukewarm about GPUStorageTextureLayoutEntry being separate but I think either way is fine. unfilterable should probably non-filterable so that it matches non-filtering but I understand I might have a higher tolerance to broken english than others :P

GPUTextureComponentType should be renamed because it doesn't really explain what depth-comparison it is. Instead we could rename it to GPUSamplingOperationType or GPUSamplingOperationResult (the result is a depth-comparison or the result if a float value)

toji · 2020-11-17T18:55:06Z

unfilterable should probably non-filterable so that it matches non-filtering but I understand I might have a higher tolerance to broken english than others :P

unfilterable sounds ever-so-slightly more natural to me than non-filterable, but both sound a bit awkward and neither is so outright wrong that I'd balk at it. #NamingIsHard

kainino0x · 2020-11-17T19:02:05Z

unfilterable sounds ever-so-slightly more natural to me than non-filterable, but both sound a bit awkward and neither is so outright wrong that I'd balk at it. #NamingIsHard

Same, both are ok with me.

kainino0x · 2020-11-17T19:05:16Z

GPUTextureComponentType should be renamed because it doesn't really explain what depth-comparison it is. Instead we could rename it to GPUSamplingOperationType or GPUSamplingOperationResult (the result is a depth-comparison or the result if a float value)

We were going to rename it to depth-stencil because it's not comparison-only; you should also be able to read from depth/stencil. (I can't remember where the conversation with @kvark about this was, maybe only in meetings.) Then calling it component type makes sense.

kainino0x · 2020-11-17T19:17:32Z

spec/index.bs

+enum GPUTextureType {
+    "filterable",
+    "unfilterable",
+    "multisample",


idea: since they others describe what can be done ("-able") maybe this should be the same? e.g. "sample-addressable" or "multisample-addressable"

kvark · 2020-11-17T22:56:19Z

@kainino0x that was in #1198 (comment)

kainino0x · 2020-11-18T00:01:35Z

I filed #1230. We can fix it in or after this PR, either way.

kvark

Amazing work here, thank you @toji for iterating on this!

kvark · 2020-11-20T16:10:30Z

spec/index.bs

@@ -1039,14 +1037,14 @@ a better limit is not specified.
    <tr><td><dfn>maxUniformBufferBindingSize</dfn>
        <td>{{GPUSize32}} <td>Higher <td>16384
    <tr class=row-continuation><td colspan=4>
-        The maximum {{GPUBufferBinding}}.{{GPUBufferBinding/size}} for bindings
-        of type {{GPUBindingType/uniform-buffer}}.
+        The maximum {{GPUBufferBinding}}.{{GPUBufferBinding/size}} for bindings for which the


need to file an issue (if we haven't already) to specify that this applies to the effective size, not given size. I.e. the size can be undefined

spec/index.bs

kvark · 2020-11-20T20:20:31Z

spec/index.bs

+<script type=idl>
+enum GPUTextureType {
+    "filterable",
+    "unfilterable",


I think there is a problem with this approach. When using non-float textures, it would be nice if the users could rely on the default GPUTextureType and not specify it, because clearly it's redundant. Non-float textures can't be sampled in WGSL, so they naturally aren't filterable (and this term shouldn't even apply...). So we can't default this to filterable.

How about this strawman:

enum GPUTextureType { "float", "unfilterable-float", "depth-float", "sint", "uint", }; dictionary GPUTextureLayoutEntry { GPUTextureType type = "float"; GPUTextureViewDimension viewDimension = "2d"; bool multisampled = false; };

Names are subject to change, of course :) The benefit of this is no need to specify redundant info for integer textures.

Shouldn't be a problem, but I'll leave it to others a bit better versed in the underlying APIs to greenlight this before I make the change.

+1 to @kvark's suggestion. I think we recently (re-)realized that D3D12 prevented any form of sampling of integer textures so the filterable / unfilterable distinction is only for float textures.

The names depth-float and GPUTextureType seem a bit generic though, so I'd be open to bikeshed them. (though that could be in a follow-up if this is forcing too many iterations)

spec/index.bs

kainino0x · 2020-11-21T03:07:40Z

I think this is on the main meeting agenda, but speculatively adding to the editors meeting agenda too.

kvark

Thank you for updating!
The last concern about GPUTextureType still stands (we'll discuss it), but everything else looks great.

toji · 2020-11-23T20:42:41Z

Updated to apply @kvark's suggestion after the structure of it was agreed on in the call (% a bit more name bikeshedding.)

kvark

GPUTextureComponentType should now be removed

kainino0x · 2020-11-23T22:58:15Z

spec/index.bs

    <tr>
        <td>{{GPUTextureFormat/r32float}}
-        <td>{{GPUTextureComponentType/"float"}}
+        <td>{{GPUTextureSampleType/"unfilterable-float"}}
        <td>&checkmark;
        <td>&checkmark;
        <td><!-- Metal -->


Extra td (this comment should move next to "unfilterable-float"

kainino0x · 2020-11-23T23:05:02Z

spec/index.bs

  "sint",
  "uint",
 };

 dictionary GPUTextureBindingLayout {
-    GPUTextureType type = "float";
+    GPUTextureSampleType type = "float";


Should we call this field sampleType or something?

That would break the pattern of every other binding layout member having a type, which is a nice bit of consistency. FWIW We currently have the type member of GPUStorageTextureBindingLayout be a GPUStorageTextureAccess enum, so it's already not a 1:1 enum/member name mapping.

kainino0x · 2020-11-23T23:06:10Z

spec/index.bs

@@ -2808,7 +2808,7 @@ A {{GPUBindGroupLayout}} object has the following internal slots:
                                    - |textureLayout|.{{GPUTextureBindingLayout/viewDimension}} is
                                        {{GPUTextureViewDimension/"2d"}}.
                                    - |textureLayout|.{{GPUTextureBindingLayout/type}} is not
-                                        {{GPUTextureType/"depth-float"}}.
+                                        {{GPUTextureSampleType/"depth"}} or {{GPUTextureSampleType/"float"}}.


I'm just (re-)noticing this particular rule. Shouldn't it be possible to load samples out of a multisampled depth texture? The rule may have been added when we thought "depth-comparison" bindings could be used only for comparison and not for loading (EDIT: and that depth textures could be bound as "float").

This can be a followup but let's make sure to file an issue if we land with this outstanding.

I wondered about that too after today's call. I'm happy to remove the depth case here.

spec/index.bs

kainino0x · 2020-11-24T02:25:24Z

OK, this is excessively detailed, but here's a spreadsheet of the possible classes of textures and how you would use them in a layout.
https://docs.google.com/spreadsheets/d/12u6g6-dUGxuwgaNVRDdZVZ0u2zb9kotqEO3_ZA-1930/edit?usp=sharing
(As usual let me know if the spreadsheet is wrong.)

Note there's a new, smaller problem similar to the one @kvark identified (#1223 (comment)) which is that if you set multisampled:true, then leaving the GPUTextureSampleType as default ("float") is invalid (multisampled:true,type:"float"). We could make it valid if we want, though, and it would have exactly the same meaning as multisampled:true,type:"unfilterable-float".

kainino0x · 2020-11-24T02:26:17Z

tl;dr I think what we have now is fine, but maybe consider making (true,"float) valid and definitely consider cases we want to support in the future

kvark

I think the changes to the table are not complete. First, there is still this sentence to be removed:

The "Filter" column specifies whether textures of this format can be sampled in shaders with a sampler having ...

Secondly, for filterable formats we want to list both "float" and "unfilterable-float" as supported sample types.

toji · 2020-11-24T22:14:52Z

I think the changes to the table are not complete...

Done!

kvark

Thank you!

1047: Update bind group layout API to match upstream r=cwfitzgerald a=kvark **Connections** Follows gpuweb/gpuweb#1076, gpuweb/gpuweb#1223 (gpuweb/gpuweb#1164), gpuweb/gpuweb#1255, and gpuweb/gpuweb#1256 **Description** Aligns our API closer to the latest changes in WebGPU upstream. We technically don't have to do this, but I believe in the end it would be best if our API gets close to upstream. Note: this is a sensitive change for the users, everybody will get their code broken. So please take a look at the API and see if something is missing or needs improvement, so that we don't have to go through the changes again afterwards. **Testing** Doesn't really need testing. Partially covered by the existing playtest. Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

1331: Validate filtering r=cwfitzgerald a=kvark **Connections** See gpuweb/gpuweb#1223 **Description** This PR implements validation of sampler filtering (property in BGL), as well as the ability to filter non-filterable textures. **Testing** Untested Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

This PR adds unimplemented stubs for the `atanh` builtin. Issue: gpuweb#1223

Issue gpuweb#1223

toji requested review from kvark and kainino0x November 14, 2020 01:06

kainino0x reviewed Nov 14, 2020

View reviewed changes

kainino0x added the for webgpu editors meeting label Nov 16, 2020

kainino0x removed the for webgpu editors meeting label Nov 16, 2020

toji marked this pull request as ready for review November 17, 2020 05:28

kainino0x reviewed Nov 17, 2020

View reviewed changes

Kangz mentioned this pull request Nov 20, 2020

Case for optional GPUBindGroupLayoutEntry entries #851

Open

kvark reviewed Nov 20, 2020

View reviewed changes

kainino0x added the for webgpu editors meeting label Nov 21, 2020

kvark approved these changes Nov 23, 2020

View reviewed changes

kvark requested changes Nov 23, 2020

View reviewed changes

kainino0x removed the for webgpu editors meeting label Nov 23, 2020

kainino0x approved these changes Nov 23, 2020

View reviewed changes

toji added 3 commits November 23, 2020 16:32

Refactoring GPUBindGroupLayout to isolate binding type definitions

c1a9a66

Update based on feedback and fixed remaining references

54b2f6b

Address feedback from @kvark

ce36e66

toji added 5 commits November 23, 2020 16:32

Apply GPUTextureBindingLayout change suggested by @kvark

1cce233

Addressing topics discussed on editors call

6450878

Remove GPUTextureComponentType

1d23f29

Feedback from @kainino0x

330d544

Rebase to resolve build errors

d19a5ac

toji force-pushed the bind-group-layout-refactor branch from afdcf6d to d19a5ac Compare November 24, 2020 00:34

kainino0x reviewed Nov 24, 2020

View reviewed changes

spec/index.bs Show resolved Hide resolved

kvark reviewed Nov 24, 2020

View reviewed changes

Address @kvark's feedback

4cbd93e

kvark approved these changes Nov 24, 2020

View reviewed changes

kvark merged commit f8f9ed9 into gpuweb:main Nov 24, 2020

toji deleted the bind-group-layout-refactor branch November 24, 2020 23:42

This was referenced Nov 25, 2020

Rename binding types #1256

Merged

Update bind group layout API to match upstream gfx-rs/wgpu#1047

Merged

Add filtered texture and sampler binding types #1076

Closed

kainino0x mentioned this pull request Nov 30, 2020

Rename "depth-comparison" to "depth-stencil" #1230

Closed

kvark mentioned this pull request Jan 4, 2021

Separate linear sampler binding type? #1034

Closed

kvark mentioned this pull request Apr 14, 2021

Validate filtering gfx-rs/wgpu#1331

Merged

kainino0x mentioned this pull request Aug 24, 2022

sampleType: "float" is not valid with multisampled: true #3364

Closed

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

wgsl: stub tests for atanh builtin. (gpuweb#1314)

7eec30c

This PR adds unimplemented stubs for the `atanh` builtin. Issue: gpuweb#1223

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

Implement atanh f32 tests (gpuweb#1741)

727a391

Issue gpuweb#1223

Refactor GPUBindGroupLayout to isolate binding type definitions #1223

Refactor GPUBindGroupLayout to isolate binding type definitions #1223

Conversation

toji commented Nov 14, 2020 • edited by pr-preview bot

kainino0x 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

Kangz commented Nov 16, 2020

kainino0x commented Nov 16, 2020

kainino0x commented Nov 16, 2020

kainino0x commented Nov 16, 2020

toji commented Nov 17, 2020

Kangz commented Nov 17, 2020

toji commented Nov 17, 2020

kainino0x commented Nov 17, 2020

kainino0x commented Nov 17, 2020

Choose a reason for hiding this comment

kvark commented Nov 17, 2020

kainino0x commented Nov 18, 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

kainino0x commented Nov 21, 2020

kvark left a comment

Choose a reason for hiding this comment

toji commented Nov 23, 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

kainino0x Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x commented Nov 24, 2020 • edited

kainino0x commented Nov 24, 2020

kvark left a comment

Choose a reason for hiding this comment

toji commented Nov 24, 2020

kvark left a comment

Choose a reason for hiding this comment

toji commented Nov 14, 2020 •

edited by pr-preview bot

kainino0x Nov 23, 2020 •

edited

kainino0x commented Nov 24, 2020 •

edited