Reduce max binding index and make it a limit #3318
Conversation
spec/index.bs
Outdated
| - Let |limits| be |this|.{{GPUObjectBase/[[device]]}}.{{device/[[limits]]}}. | ||
| - The {{GPUBindGroupLayoutEntry/binding}} of each entry in |descriptor| is unique. | ||
| - The {{GPUBindGroupLayoutEntry/binding}} of each entry in |descriptor| must be < | ||
| |limits|.{{supported limits/maxBindingIndex}}. |
There was a problem hiding this comment.
the name maxBindingIndex makes it sound like it should be a "≤". "≤" is probably fine, I don't have a good alternative name suggestion.
There was a problem hiding this comment.
I don't have a great name suggestion either that would allow it to continue being a "<". It's a little weird for it to be a "≤", since that means technically we would allow 641 unique binding indices, but given that it's something of an in-joke anyway I don't think that matters. In the future if we have a "real" limit it won't look that weird to have some square number-1, as most devs will recognize where that comes from. (Plus we use similar verbiage for things like maxComputeWorkgroupSize* so it's not completely without precedent.)
There was a problem hiding this comment.
If we really wanted it to be 640 we could maybe name it maxBindingsPerBindGroup, it would just be slightly misleading because you can never actually have that many bindings per bind group (other limits are the limiting factor). I'm fine with 641. shrug
7079e8d to
ed2972c
Compare
|
Previews, as seen when this build job started (ed2972c): |
|
Realized one thing we're missing from the last meeting notes:
We should guarantee this as well. |
|
I don't remember from that conversation and the meeting notes aren't enlightening. What is this1, this2, and that in this context? |
|
Basically I think we want to say, for the limit values exposed on an adapter, it must be the case that |
|
Ah, that makes sense. I've updated the PR to apply what I think is the right logic there. PTAL. |
| readonly attribute unsigned long maxBindGroups; | ||
| readonly attribute unsigned long maxBindingsPerBindGroup; | ||
| readonly attribute unsigned long maxDynamicUniformBuffersPerPipelineLayout; | ||
| readonly attribute unsigned long maxDynamicStorageBuffersPerPipelineLayout; |
There was a problem hiding this comment.
unrelated aside, I guess these should be ≤ (the corresponding per shader stage limits * 2)?
|
Previews, as seen when this build job started (e920880): |
|
Meeting: No concerns surfaced with name, will land Monday if no further input. |
Fixes #3279. Uses the suggested limit of 640. Also took the opportunity to update the timeline style of the affected algortihm, which I'll probably be doing for most of my future PRs.
Co-authored-by: Kai Ninomiya <kainino@chromium.org>
51d5d4f to
0641500
Compare
|
Previews, as seen when this build job started (0641500): |
|
It's Monday and no new comments have been made, so landing. |
|
Removing 'needs-cts-label' as this is updated as a part of the draft CTS named |
|
@lokokung fyi generally speaking for changes like this there could be other tests that need updating too, but in this case looks like we already updated them. (There's one MAINTENANCE_TODO to move a constant into |
Fixes #3279.
Uses the suggested limit of 640, with an annotation to help out those who don't
get where that value comes from. Also took the opportunity to update the
timeline style of the affected algortihm, which I'll probably be doing for most
of my future PRs.