wgsl: replace stride attribute with array type parameter by dneto0 · Pull Request #2103 · gpuweb/gpuweb

dneto0 · 2021-09-13T22:48:18Z

Update related grammar rules and examples
Rename 'runtime-sized' array to 'dyanmically-sized'.
Change its declaration syntax to use dynamic_array instead of
array without an element count.
Add examples showing array type compatibility based on stride

Editorial:

Rearrange text in Array Types section
More consistently use 'E' for element array type placeholder

Part of #1534

Change-Id: I09082c257fbc325e20a0e28fe16f7744e882a61e

dneto0 · 2021-09-13T22:48:43Z

We're very open to a name that's better than dynamic_array (!)

github-actions · 2021-09-13T22:53:24Z

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

kvark

(haven't finished the review)

wgsl/index.bs

kvark · 2021-09-14T00:45:13Z

wgsl/index.bs

+
+    // array<i32,5,4> and array<i32,5> are the same type because the implicit stride of
+    // array<i32,5> is 4.
+    var<private> g: array<i32,5,4>;


unfortunately, this doesn't read well :(

Alternative syntaxes have been proposed and rejected, like

array<i32,5,stride=4>

Do you have another alternative?

How about:

// Both the g and h arrays instantiate the same type because h's implicit stride
// is the same as g's explicit stride.
var g : array<i32,5,4>; // Array of 5 i32s with a stride of 4-bytes
var h : array<i32,5>; // Array of 5 i32s with an implicit stride of 4-bytes

I don't have a good suggestion here. Enrolling into named generic parameters is certainly one way, but it's a lot to chew for this (seemingly little) problem. Leaving <i32, 5, 4> is just looking confusing.

From group discussion on 2021-10-12, we decided to try the 'stride=N' syntax.
Latest commit shows this.

dneto0 · 2021-09-29T18:00:15Z

Rebased against main.

github-actions · 2021-09-29T18:01:04Z

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

kdashg · 2021-10-12T19:02:09Z

WGSL meeting minutes 2021-10-12

DN: Part of overall attempt to remove attributes from types. Make the stride part of the array template type. But then spell fixed-size and runtime-sized array differently. Looking for high level approval of whether this is a good direction. Can bikeshed details later.
MM to ensure existence of an issue for not having stride
DM: I think we should probably not move stride around, if we might remove it, and so we might want to block on it.
MM: I think that’s perfect-as-enemy-of-good, and we should just take this and make other changes later.
DM: I still don’t like the array<i32,5,4> number soup syntax
DN: We could do the python-like thing for a named argument for stride.
DN: Do we like having a named arg?
MM: My knee-jerk reaction is
DM: I think naming with named args is great, but that it’s a larger grammar change that I’d like to avoid. Basically a new language feature
DN: If I were to start from scratch within the param list, I would probably not split spelling a dynamic-vs-fixed array. Would that be better than what’s on the table right now?
DM: Yes
MM: If allowing a named stride, would be cool to have a named length too.
JG: Aren’t there non-grammatical differences between fixed-vs-dynamic? I like spelling things different if they behave differently.
DN: Ok yeah they do have differences, I see your point there.
DM: Since this problem might go away if MM gets us to get rid of strides, which lets us ship with fewer changes. Shouldn’t we block on that?
MM: Yeah I can put up that issue so we can talk about that first.

- Update related grammar rules and examples - Rename 'runtime-sized' array to 'dyanmically-sized'. Change its declaration syntax to use `dynamic_array` instead of `array` without an element count. - Add examples showing array type compatibility based on stride Editorial: - Rearrange text in Array Types section - More consistently use 'E' for element array type placeholder Part of gpuweb#1534 Change-Id: I09082c257fbc325e20a0e28fe16f7744e882a61e

dneto0 · 2021-10-13T12:39:54Z

685ed9e is rebased against main.

- Add new 'modifier word' kind of token which has special meaning only in certain contexts, and is otherwise is an identifier. Note: We should probably make lots of things like this: access mode, image format - When stride is specified in source, spell it out as ',stride=N' instead of just an optional ',N' - Modify the grammar rules for array type declarations, to match the prose.

github-actions · 2021-10-13T13:48:05Z

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

github-actions · 2021-10-13T13:50:37Z

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

kdashg · 2021-10-20T05:54:54Z

WGSL meeting minutes 2021-10-19

Previously:
- MM to ensure existence of an issue for not having stride
- MM: Yeah I can put up that issue so we can talk about that first.
Updates:
- Still:
  - spell runtime-sized array as ‘dynamic_array’.
- New:
  - Introduced ‘modifier word’ kind of token, which has special meaning only in certain contexts and is otherwise interpreted as an identifier. (Should make ‘access mode’ and image formats into these, rather than keywords.)
  - Use ‘,stride=N’ syntax instead of ‘,N’ to specify explicit stride.
  - Updated grammar to match the prose.
Block on MM’s TBD issues

* Remove two paragraphs that are part of gpuweb#2103, the array-stride change.

kdashg · 2022-01-24T23:47:53Z

Is this still valid? @dneto0

Kangz · 2022-02-01T18:10:55Z

Closing since strides have been removed.

dneto0 added the wgsl WebGPU Shading Language Issues label Sep 13, 2021

dneto0 added this to the V1.0 milestone Sep 13, 2021

dneto0 requested review from alan-baker, kvark and litherum September 13, 2021 22:48

This was referenced Sep 14, 2021

wgsl: Sketch design for arrays of buffer-bindings #2105

Open

Remove block attribute #2104

Merged

kvark reviewed Sep 14, 2021

View reviewed changes

dneto0 force-pushed the issue-1534-remove-stride branch from d454ed4 to fc2d250 Compare September 29, 2021 17:54

dneto0 force-pushed the issue-1534-remove-stride branch from fc2d250 to 685ed9e Compare October 13, 2021 12:39

dneto0 added 3 commits October 13, 2021 08:40

Resolve issue: agreed "dynamic_array" name is ok

c06ea22

Binding range of buffer determines the dynamic array size

4bd7cd7

alan-baker approved these changes Oct 19, 2021

View reviewed changes

dneto0 added a commit to dneto0/gpuweb that referenced this pull request Nov 16, 2021

Remove parts of gpuweb#2103

98d7315

* Remove two paragraphs that are part of gpuweb#2103, the array-stride change.

dneto0 added a commit to dneto0/gpuweb that referenced this pull request Nov 16, 2021

Remove more stray material from gpuweb#2103

a49f5d7

dneto0 added a commit to dneto0/gpuweb that referenced this pull request Nov 25, 2021

Remove parts of gpuweb#2103

3887339

* Remove two paragraphs that are part of gpuweb#2103, the array-stride change.

dneto0 added a commit to dneto0/gpuweb that referenced this pull request Nov 25, 2021

Remove more stray material from gpuweb#2103

d25f7f2

Kangz closed this Feb 1, 2022

Conversation

dneto0 commented Sep 13, 2021

Uh oh!

dneto0 commented Sep 13, 2021

Uh oh!

github-actions bot commented Sep 13, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kvark Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

dneto0 Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

alan-baker Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

dneto0 Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

dneto0 commented Sep 29, 2021

Uh oh!

github-actions bot commented Sep 29, 2021

Uh oh!

kdashg commented Oct 12, 2021

Uh oh!

dneto0 commented Oct 13, 2021

Uh oh!

github-actions bot commented Oct 13, 2021

Uh oh!

github-actions bot commented Oct 13, 2021

Uh oh!

kdashg commented Oct 20, 2021

Uh oh!

kdashg commented Jan 24, 2022

Uh oh!

Kangz commented Feb 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alan-baker Sep 29, 2021 •

edited

Loading