Store type for buffer does not have to be structure#2401
Merged
dneto0 merged 7 commits intogpuweb:mainfrom Jan 19, 2022
Merged
Conversation
Contributor
Author
|
cc: @ben-clayton @Kangz |
dneto0
commented
Dec 8, 2021
wgsl/index.bs
Outdated
|
|
||
| // Uniform buffer. Always read-only, and has more restrictive layout rules. | ||
| struct ParamsTable {}; | ||
| struct ParamsTable {gain: f32;}; |
Contributor
Author
There was a problem hiding this comment.
Structures always have at least one member. Fix this example.
Contributor
Contributor
|
This is great! |
alan-baker
approved these changes
Dec 9, 2021
Contributor
|
Just because we can support non-structures doesn't mean we should. Is this supported by anything except Metal in lower level APIS? There is a few of us feeling that WGSL would be a better language if it only allows structures and arrays in this position, not arbitrary types. #2188 (comment) |
alan-baker
requested changes
Dec 15, 2021
Contributor
alan-baker
left a comment
alan-baker
left a comment
There was a problem hiding this comment.
I realize this likely needs a change in the WebGPU spec for GPUProgrammableShaderStage validation. That validation explicitly refers to structures for the WGSL variable.
dneto0
force-pushed
the
issue-2188-buffer-non-struct
branch
from
64aace6 to
5fc2495
Compare
Contributor
Author
|
Only a rebase for now... |
Contributor
Contributor
alan-baker
requested changes
Jan 12, 2022
kvark
requested changes
Jan 12, 2022
Contributor
dneto0
force-pushed
the
issue-2188-buffer-non-struct
branch
from
a8efbee to
8833b30
Compare
Contributor
Contributor
Contributor
jrprice
approved these changes
Jan 18, 2022
kvark
approved these changes
Jan 18, 2022
* Modify an example showing a runtime-sized array as the store type for a storage buffer. Fixes: gpuweb#2188
The store type of the corresponding variable is not always going to be a structure type. Qualify the rule accordingly.
Define 'minimum binding size' in WGSL spec, and link to it from WebGPU. Repeat the rule in both places (to be helpful). The minimum binding size for a var with store type |T| is max(AlignOf(T),SizeOf(T)), and explain why the AlignOf part is needed: it's because sometimes we have to wrap |T| in a struct. This also replaces the old rule in WGSL which confusingly dependend on the storage class. The storage class aspect is already embedded in the alignment and size constraints for the variable.
Underlying APIs don't need the extra padding at the end of any structure which might wrap the store type for the buffer variable.
dneto0
force-pushed
the
issue-2188-buffer-non-struct
branch
from
2f834e6 to
bf408ae
Compare
Contributor
Author
|
I did a rebase and resolve conflicts. |
Contributor
- Link to SizeOf in the WGSL spec - More carefully describe the motivation for the min-binding-size constraint.
Contributor
"Mapping" is how the buffer's storage is paged into host-accessible address space. That's a different concept entirely, and would only confuse things.
Contributor
RobinMorisset
pushed a commit
to RobinMorisset/gpuweb
that referenced
this pull request
Jan 24, 2022
* Store type for buffer does not have to be structure * Modify an example showing a runtime-sized array as the store type for a storage buffer. Fixes: gpuweb#2188 * Update the API-side rules about minBindingSize The store type of the corresponding variable is not always going to be a structure type. Qualify the rule accordingly. * Rework minimum binding size in both WebGPU and WGSL spec Define 'minimum binding size' in WGSL spec, and link to it from WebGPU. Repeat the rule in both places (to be helpful). The minimum binding size for a var with store type |T| is max(AlignOf(T),SizeOf(T)), and explain why the AlignOf part is needed: it's because sometimes we have to wrap |T| in a struct. This also replaces the old rule in WGSL which confusingly dependend on the storage class. The storage class aspect is already embedded in the alignment and size constraints for the variable. * Simplify minimum binding size to Sizeof(store-type) Underlying APIs don't need the extra padding at the end of any structure which might wrap the store type for the buffer variable. * Update API-side to SizeOf(store-type) * Apply review feedback - Link to SizeOf in the WGSL spec - More carefully describe the motivation for the min-binding-size constraint. * Simplify, and avoid using the word "mapping" "Mapping" is how the buffer's storage is paged into host-accessible address space. That's a different concept entirely, and would only confuse things.
RobinMorisset
pushed a commit
to RobinMorisset/gpuweb
that referenced
this pull request
Jan 24, 2022
* Store type for buffer does not have to be structure * Modify an example showing a runtime-sized array as the store type for a storage buffer. Fixes: gpuweb#2188 * Update the API-side rules about minBindingSize The store type of the corresponding variable is not always going to be a structure type. Qualify the rule accordingly. * Rework minimum binding size in both WebGPU and WGSL spec Define 'minimum binding size' in WGSL spec, and link to it from WebGPU. Repeat the rule in both places (to be helpful). The minimum binding size for a var with store type |T| is max(AlignOf(T),SizeOf(T)), and explain why the AlignOf part is needed: it's because sometimes we have to wrap |T| in a struct. This also replaces the old rule in WGSL which confusingly dependend on the storage class. The storage class aspect is already embedded in the alignment and size constraints for the variable. * Simplify minimum binding size to Sizeof(store-type) Underlying APIs don't need the extra padding at the end of any structure which might wrap the store type for the buffer variable. * Update API-side to SizeOf(store-type) * Apply review feedback - Link to SizeOf in the WGSL spec - More carefully describe the motivation for the min-binding-size constraint. * Simplify, and avoid using the word "mapping" "Mapping" is how the buffer's storage is paged into host-accessible address space. That's a different concept entirely, and would only confuse things.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
for a storage buffer.
Fixes: #2188
Preview | Diff