Change `offset` to `span`. #1339

dj2 · 2021-01-07T21:31:39Z

This CL removes the offset decoration from struct members and adds a
span decoration instead. The span reduces the amount of action at a
distance required when adding data members as each line is self
contained. This also reduces the amount of typing required as the span
is only required when the memory size does not patch the type size for a
member.

Fixes #1303

This CL removes the offset decoration from struct members and adds a `span` decoration instead. The span reduces the amount of action at a distance required when adding data members as each line is self contained. This also reduces the amount of typing required as the span is only required when the memory size does not patch the type size for a member. Fixes #1303

dj2 · 2021-01-07T21:32:16Z

First pass at converting over to span. The change itself is already approved by the group, so once the text is good we should be good to land.

github-actions · 2021-01-07T21:33:39Z

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

kvark · 2021-01-07T21:44:14Z

wgsl/index.bs

 * [[#matrix-types]]
 * [[#array-types]] if the array type has a [=stride=] attribute and its element type is host-shareable
-* [[#struct-types]] if each member is host-shareable and has an [=offset=] attribute
+* [[#struct-types]] if each member is host-shareable and, if the size of the member isn't the same as the size of the type, has a [=span=] attribute


This is a little bit confusing: "the size of the member isn't the same as the size of the type". Are we really talking about the size of the member here? Or maybe about it's memory span?

Actually, we should have a separate definition somewhere, defining a set of types that match this description

Memory span. Number of bytes taken up by the thing in memory. Will attempt to clarify.

kvark · 2021-01-07T21:46:03Z

wgsl/index.bs

-         The number of bytes between the start of the structure and the location
-         of this member.
+         In the case where the memory size of a member does not match the size of the member type, a
+         `span` is required. The `span` must be the byte size of memory taken up by the current member and no larger.


Should we mention somewhere what values of the span are expected? This text says it must be "the byte size of memory taken up by the current member" but doesn't explain how to get that.

Maybe? I think we can skip it for now, but this maybe an output from the compilers, or a table to show when needed?

wgsl/index.bs

litherum · 2021-01-09T07:39:14Z

This CL removes the offset decoration

Why not make it optional instead? If an author for some reason wants to tell us the offset they expect stuff to be at, we ought to be receptive of that information.

If we make it optional, we'd have to make offset and span not conflict. E.g. you should be able to do

...
[[offset(16) span(16)]] metallicRoughnessFactor : vec2<f32>;
emissiveFactor : vec2<f32>;
...

dj2 · 2021-01-11T14:14:10Z

I think we should leave offset out of the MVP and revisit if there is call for it afterwards. It's more spec work and more implementation work to add it in, and we don't know if it will be necessary.

github-actions · 2021-01-11T20:59:26Z

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

kainino0x · 2021-01-12T19:24:06Z

Comment from meeting: Instruct { a: vec2<f32>; b: vec4<f32>; }, vec2 has a "natural size" of 8 bytes, but would require a span of 16 in order to align the vec4, so it would have to be explicit ([[span(16)]] a: vec2, b: vec4). I don't think we want to add automatic alignment rules so that vec2 implicitly gets a span of 16. If there were an explicit annotation to align (a: vec2, [[align(16)]] b: vec4), required any time that implicit padding would be inserted due to alignment rules, then maybe it would be okay. (Autogenerated code could unconditionally add [[align()]] annotations and then would never break due to the order of fields changing.)
This also means span info is context dependent and so it makes most sense to have it on the member, not on the type.

kvark · 2021-01-20T00:33:49Z

Doing my share of work to help this case, I tried to crystallize the debate we had so far. Here are the plain and graph views. I'm definitely missing a lot of arguments here (was reconstructing from memory, need to re-read all the discussions, ideally, to populate it), since it appears to me that just slapping [[layout(general)]] or [[layout(storage)]] on textures (related to #621) is the simplest solution. Any help is appreciated!

dj2 · 2021-01-20T14:12:17Z

I think the concern with add the layout is the number of possible entries getting large? layout(140), layout(430), layout(scalar), layout(relaxed) ....

kvark · 2021-01-20T14:42:43Z

@dj2 right, but it's not like these layouts are magically going away if we aren't explicit about them. If we support "scalar" at any point, we'd still have to specify it in terms of constraints on the offsets/spans/whatever, i.e. it will technically be a part of the spec, so it might as well be just layout(scalar). This is probably not the best place to have this discussion, sorry for de-railing! I suppose we can chat in #621 if there is any follow-up, instead.

Fixes gpuweb#1339

dj2 added the wgsl WebGPU Shading Language Issues label Jan 7, 2021

dj2 added this to the MVP milestone Jan 7, 2021

dj2 requested review from dneto0 and kvark January 7, 2021 21:31

dj2 self-assigned this Jan 7, 2021

kvark reviewed Jan 7, 2021

View reviewed changes

review updates

9339d41

kvark mentioned this pull request Jan 19, 2021

[wgsl] Redundant [[layout]] decorations in addition to [[offset]] decorations #621

Closed

kvark mentioned this pull request Jan 22, 2021

Replace [[block]] attribute by [[layout(...)]] #1361

Closed

dneto0 mentioned this pull request Jan 22, 2021

conflict between mat2x2 layout in uniform buffer and storage buffer #1258

Closed

dj2 closed this Mar 1, 2021

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

Handle backends flushing inputs in atan2 tests (gpuweb#1340)

d5cf9ab

Fixes gpuweb#1339

Change offset to span. #1339

Change offset to span. #1339

Uh oh!

Conversation

dj2 commented Jan 7, 2021

Uh oh!

dj2 commented Jan 7, 2021

Uh oh!

github-actions bot commented Jan 7, 2021

Uh oh!

kvark Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

dj2 Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

dj2 Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

dj2 Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

litherum commented Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dj2 commented Jan 11, 2021

Uh oh!

github-actions bot commented Jan 11, 2021

Uh oh!

kainino0x commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Jan 20, 2021

Uh oh!

dj2 commented Jan 20, 2021

Uh oh!

kvark commented Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Change `offset` to `span`. #1339

Change `offset` to `span`. #1339

litherum commented Jan 9, 2021 •

edited

Loading

kainino0x commented Jan 12, 2021 •

edited

Loading