[WGSL] Default struct memory layout #1447
Conversation
kvark
left a comment
kvark
left a comment
There was a problem hiding this comment.
Very nice, thank you for writing this!
Personally I was surprised that an array alignment is not the stride, so TIL.
| <td>[=roundUp=](16, |A|),<br> where |A| = max(|Align|(|T1|,`uniform`),..., |Align|(|Tn|,`uniform`))) | ||
| <td>struct<T<sub>0</sub>, ..., T<sub>N</sub>> | ||
| <td>max([=AlignOf=](T<sub>0</sub>), ..., [=AlignOf=](T<sub>N</sub>)) | ||
| <td>[=roundUp=](16, max([=AlignOf=](T<sub>0</sub>), ..., [=AlignOf=](T<sub>N</sub>)))<br> |
There was a problem hiding this comment.
I know I suggested this way, but looking at it again I wonder if defining it in terms of storage is better (for uniform)
roundUp(16, RequiredAlignOf(S, storage))
I'm not sure it is, but I leave it to your discretion.
There was a problem hiding this comment.
I'm not sure any more.
This had me scratching my head a bit to make sure the values were actually identical to what was there before, but as 16 bytes is the largest required alignment of any type, I think this is the same.
In terms of reading the spec, I feel what we currently have is easier to understand, but I don't know if we're folding away too much that'll bite us if / when rules change.
There was a problem hiding this comment.
I think it boils down to 'roundup(k, ...)' distributes over max, which I believe is true .
Attempting to implement this had me jumping around in the spec
ben-clayton
changed the title
|
Good for final review. |
dneto0
left a comment
dneto0
left a comment
There was a problem hiding this comment.
I think my feedback is almost purely editorial,
except for the "array stride must be a multiple of element alignment"
| Alignment is a function of both the type and the [=storage class=] of the buffer. | ||
| All structure and array types directly or indirectly referenced by a variable | ||
| must obey the constraints of the variable's storage class. | ||
| Violations of a storage class constraint result in a compile-time error. |
There was a problem hiding this comment.
Note: We may tweak this wording once #1241 is fixed.
| <td>[=roundUp=](16, |A|),<br> where |A| = max(|Align|(|T1|,`uniform`),..., |Align|(|Tn|,`uniform`))) | ||
| <td>struct<T<sub>0</sub>, ..., T<sub>N</sub>> | ||
| <td>max([=AlignOf=](T<sub>0</sub>), ..., [=AlignOf=](T<sub>N</sub>)) | ||
| <td>[=roundUp=](16, max([=AlignOf=](T<sub>0</sub>), ..., [=AlignOf=](T<sub>N</sub>)))<br> |
There was a problem hiding this comment.
I think it boils down to 'roundup(k, ...)' distributes over max, which I believe is true .
|
Merge when ready! |
|
I think remaining things are editorial:
|
dneto0
left a comment
dneto0
left a comment
There was a problem hiding this comment.
I've logged the editorial items to follow up.
Implements gpuweb/gpuweb#1447 SPIR-V Reader is still TODO, but continues to function as the offset decoration is still supported. Bug: tint:626 Bug: tint:629 Change-Id: Id574eb3a5c6729559382812de37b23f0c68fd406 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/43640 Commit-Queue: Ben Clayton <[email protected]> Reviewed-by: David Neto <[email protected]>
7 participants
Ready for final review