Set a limit for maximum array size by alan-baker · Pull Request #3078 · gpuweb/gpuweb

alan-baker · 2022-06-20T20:41:15Z

Set a limit of 64k bytes for an array size

github-actions · 2022-06-20T20:49:45Z

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

ben-clayton · 2022-06-21T08:45:45Z

    <tr><td>[=Nesting depth=] of a [=composite=] type<td>255
    <tr><td>Number of [=formal parameter|parameters=] for a function<td>255
    <tr><td>Number of case selector values in a [=statement/switch=] statement<td>16383
+    <tr><td>[=Byte-size=] of an [=array=] type<td>655in35


How does this work for types that have no defined byte size? e.g. bool and abstract numerics?

I think it'd be ok to leave that based on the implementation. We could add a second limit on number of elements if that is necessary.

I suggest adding:

For this limit, assume bool has byte-size of 4.

What about abstract numerics? Suggest 8 bytes for them, given that we guarantee at least 64 bit precision.

github-actions · 2022-06-21T11:50:04Z

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

ben-clayton · 2022-06-25T18:12:58Z

64k bytes does seem small for any array. Wouldn't this error out on most medium to large storage buffers?

kdashg

A brief explanation would be ideal here, if you can! This is something we're going to wonder about in the future both "why limit?" and "why this number?"

kdashg · 2022-06-29T17:06:09Z

WGSL meeting minutes 2022-06-28

Previously: wgsl: limit layout size of a type #2118 (comment)
AB: Something for discussion, internally forced us to think a bit more.
KG: Why 64k
AB: exactly. Thought that was in notes from meeting, so started there. Do we count bool or abstract number differently? Reading notes, read as minimal maximum but implementation could choose to do more. Spec doesn't’ say that for limits. Worthwhile distinction to make, otherwise why not tie to api side buffer side so if that changes, you only change once. Should agree all implementations must support at least this but they can also support more.
JB: So, implementations can’t reject things smaller then this
AB: Right, must have at least 64k bytes of elements. Separate issue for number of elements limit which would be separate. Rephrasing limits section as must support this, and above it is implementation defined. Then pick number everyone is happy
JB: Desire is to require implementations to support this but device could reject if it doesnt’ support
AB: Right, always possible for a runtime error. Do best to compile, and have tests, but can compile where you don’t run out of memory.
JB: Basically lower bound on implementation restrictions
AB: Right. Gives users guidance of at least this amount is supported.
JB: Seems reasonable.
KG: So, it’s optional?
AB: No, must support least this array size. If you choose to support more that’s Ok. but you can’t support less then that.
DS: Comes with portability issues, yea?
AB: Well, OOM isn’t portable. If you stay within bounds it’s portable, above it you’re on your own. Implementation may warn about going above it.
KG: So, ideally these are minimal maximums. Should rename section
AB: Yes. Also do we want a separate number for bools / abstracts.
KG: If we run into it … Have we run into it?
AB: Fuzzer found bools. Abstracts haven’t been hit by fuzzer yet but assume it will.
JB: So, arrays of abstracts as intermediate values in expression and higher up gets converted to array of concrete type?
AB: Also agreed to abstract const arrays, so just declare them
JB: So, constant which use at different spots which has a different type at each use?
AB: Yup
JB: Didn’t catch that
AB: That’s true for any const. Where it’s used is where you convert.
JB: Right, just an aspect i missed. Thought it was concrete when named.
KG: With understanding calling this group minimum implementation defined maximums I think this is fine.
DN: One thing, storage buffers can have very large arrays so this limit is about a constructable value but might have storage buffer with 1 million elements but can’t load that into a let. That’s the cutoff. So, carve-out for buffer contents being larger is fine.
KG: Is that true for the actual sized storage type?
DN: Think it’s 100 or 256meg is what you can bind to a buffer. Shouldn’t be limited by this
AB: That is the minimum max on Api side
MM: We can say author can make array this big and that’s cool, but if the author makes a lot of arrays of that size it can fail
AB: Lets impl pick limit internally and just stop trying. If have lots of memory and thing is giant can just say not supported
DN: Should be able to write CTS test to create array of 64k and it should pass and behave correctly
KG: Something this big should work. Feels like 64k is not enough for everyone. Need to be able to have larger arrays for storage things. That seems clear. Otherwise ban people where if they want bigger it has to be dynamic, which is too limiting and can only be one array
DC: Is this binding size of array being passed in? Or, internal array size as well
DN: Mostly about internal array sizes
DC: So, applies to any array benign bound going in?
MM: This is different, there is a limit for that, but that’s not this. That’s an API side limit, not WGSL limit.
KG: We don’t have storage location annotation on arrays right
AB: Correct
KG: But that’s what we’re getting at here (don’t want to add one) but same way we ban ptr to things based on storage this seems similar.
AB: Can phrase as this array instantiated in the following address spaces, if that’s preferable
KG: Think it would be. It’s too clear folks will want more then 64k with arrays coming in with stuff so should already say it’s a limit for private and stuff like that. Maybe just say storage and uniform are different?
AB: Uniform is worth double checking. A fixed size array that’s really big would FXC have issues as a uniform or just local variable. Don’t know off hand.
KG: So, what words should we use for 64 k limit, not storage not uniform, private function?
AB: Private, function, workgroup. Think there is another workgroup limit. Better to phrase as here are the ones it can use
DN: Don’t want creation time array expression which isn’t in storage but should be limited by this constraint.
KG: Creation time array?
DN: Creation time ;constant of array type. Ones we allowed last week.
KG: Cool. Think we’re set there
MM: Did i overhear it’s impossible to bind something of … array without wrapping in struct?
AB: No
MM: Great.

dneto0 · 2022-08-10T17:10:34Z

+    <tr><td>Maximum [=byte-size=] of an [=array=] type instantiated in the following [=address spaces=]:
+            * [=address spaces/function=]
+            * [=address spaces/private=]
+            * [=address spaces/workgroup=]


WebGPU has a 16KB limit on workgroup storage.
Suggest making a different row for workgroup storage, use 16KB and indicate that https://gpuweb.github.io/gpuweb/#dom-supported-limits-maxcomputeworkgroupstoragesize is authoritative.

dneto0

I like the min/max phrasing.

Please update the rule for workgroup.

dneto0 · 2022-08-10T18:15:10Z

+
+            For the purposes of this limit, [=bool=] has a size of 1 byte.
+        <td>[[WebGPU#dom-supported-limits-maxcomputeworkgroupstoragesize|16384]]
+    <tr><td>Maximum number of elements in [=creation-time expression=] of [=array=] type<td>65535


This is a broken link, per the bot

Right, this is from before the rename.

dneto0 · 2022-08-10T18:16:36Z

I'm not sure the group has agreed to these actual numbers. Let's put this on the agenda to verify

Fixes gpuweb#2118 * Set a limit of 64k bytes for an array size

* Changes limits to minimum maximum style * Clarify array size byte size limit is for private, function and workgroup * specify bool is 1 byte for this limit * Add limit for maximum number of elements in a creation-time expression of array type

github-actions · 2022-08-10T18:36:53Z

Previews, as seen when this build job started (4d70db6):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

kdashg · 2022-08-17T17:20:38Z

WGSL 2022-08-16 Minutes

(PR for review.)
(DN: Are we agreed on the numbers. Wasn’t clear to me from notes from last time.)
AB: Discussed before and came back with min of max type language and break up limits slightly. Have updated PR to do that when DN last looked split out workgroup memory as api sets specific limit and link that to the API. Otherwise it is the min acceptable limit and breaks out cases in a better way. Question if folks like those numbers.
MM: Like numbers. API limits are max not min (for workgroup memory).
AB: Think that’s OK. Just means the min == max here. Links against the spec so you’d go read that for more detail.
KG: By minimum maximum what do you mean?
AB: Lowest limit that your implementation (say tint) has to compile. Or like Naga has to support that many elements in array. Above that might be a compile error. What we were saying before was it would always be an error. Hard to define those cases just right. Trying to protect against asking for silly large values (as a fuzzer does) and we’d like to cut that exploration off. These limits are practical and probably don't’ need to go above them but tooling can support if it wants.
MM: Think this is reasonable don’t understand why workgroup memory is aprt as it’s just max, seems different class.
AB: Can remove it if think it’s better. We don’t list storage buffer so maybe ok to not list workgroup
MM: Purpose of API limit is there is no device that will ever go beyond that limit. So saying it may go beyond the workgroup limit contradicts API side limit. As last time, can have 1 of these arrays but second one may fail, so kinda meaningless but don’t want it to be a blocker.
KG: PR approved. Good to land.

alan-baker added the wgsl WebGPU Shading Language Issues label Jun 20, 2022

alan-baker added this to the V1.0 milestone Jun 20, 2022

alan-baker requested review from dneto0, kdashg and litherum June 20, 2022 20:41

ben-clayton reviewed Jun 21, 2022

View reviewed changes

kdashg reviewed Jun 26, 2022

View reviewed changes

kdashg added the wgsl resolved Resolved - waiting for a change to the WGSL specification label Jun 29, 2022

dneto0 reviewed Aug 10, 2022

View reviewed changes

dneto0 requested changes Aug 10, 2022

View reviewed changes

dneto0 approved these changes Aug 10, 2022

View reviewed changes

dneto0 reviewed Aug 10, 2022

View reviewed changes

alan-baker added 7 commits August 10, 2022 14:31

Set a limit for maximum array size

d477ee2

Fixes gpuweb#2118 * Set a limit of 64k bytes for an array size

fix typo

92d74b7

Rework limits section

39cbc3a

* Changes limits to minimum maximum style * Clarify array size byte size limit is for private, function and workgroup * specify bool is 1 byte for this limit * Add limit for maximum number of elements in a creation-time expression of array type

fix missing word

faca228

fix typos

6c06fc9

changes for review

47b7ba0

fix link

4d70db6

alan-baker force-pushed the max-array-size branch from d249d58 to 4d70db6 Compare August 10, 2022 18:32

kdashg approved these changes Aug 16, 2022

View reviewed changes

kdashg merged commit 14ae0c9 into gpuweb:main Aug 16, 2022

jrprice mentioned this pull request Mar 3, 2023

[WGSL] Consider removing limit for maximum workgroup array size #3917

Closed

alan-baker deleted the max-array-size branch February 9, 2024 15:59

jrprice mentioned this pull request Feb 9, 2024

[wgsl] Assume bool is 4 bytes for array limits #4487

Merged

andyleiserson mentioned this pull request Mar 20, 2025

[naga] Enforce a maximum array size gfx-rs/wgpu#7383

Closed

Conversation

alan-baker commented Jun 20, 2022

Uh oh!

github-actions Bot commented Jun 20, 2022

Uh oh!

Uh oh!

ben-clayton Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

alan-baker Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

dneto0 Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

ben-clayton Jun 25, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 21, 2022

Uh oh!

ben-clayton commented Jun 25, 2022

Uh oh!

kdashg left a comment

Choose a reason for hiding this comment

Uh oh!

kdashg commented Jun 29, 2022

Uh oh!

dneto0 Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

dneto0 left a comment

Choose a reason for hiding this comment

Uh oh!

dneto0 Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

alan-baker Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

dneto0 commented Aug 10, 2022

Uh oh!

github-actions Bot commented Aug 10, 2022

Uh oh!

kdashg commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants