Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WGSL] Default struct memory layout #1447

Merged
merged 11 commits into from
Mar 9, 2021

Conversation

ben-clayton
Copy link
Collaborator

@ben-clayton ben-clayton commented Feb 18, 2021

Ready for final review

@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
3d9a8a3

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
bf61ed4

@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
6103bee

@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
c2faab3

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you for writing this!
Personally I was surprised that an array alignment is not the stride, so TIL.

wgsl/index.bs Show resolved Hide resolved
wgsl/index.bs Show resolved Hide resolved
wgsl/index.bs Show resolved Hide resolved
@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
82e2c39

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Show resolved Hide resolved
wgsl/index.bs Show resolved Hide resolved
@ben-clayton ben-clayton marked this pull request as ready for review March 1, 2021 22:52
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
<td>[=roundUp=](16, |A|),<br> where |A| = max(|Align|(|T1|,`uniform`),..., |Align|(|Tn|,`uniform`)))
<td>struct&lt;T<sub>0</sub>, ..., T<sub>N</sub>&gt;
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it boils down to 'roundup(k, ...)' distributes over max, which I believe is true .

@ben-clayton ben-clayton changed the title [WGSL] Begin drafting the default struct layout proposal (WIP) [WGSL] Default struct memory layout Mar 3, 2021
@ben-clayton
Copy link
Collaborator Author

Good for final review.

@kvark kvark added this to Needs Approval in WGSL Mar 3, 2021
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my feedback is almost purely editorial,
except for the "array stride must be a multiple of element alignment"

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Show resolved Hide resolved
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: We may tweak this wording once #1241 is fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

wgsl/index.bs Outdated Show resolved Hide resolved
<td>[=roundUp=](16, |A|),<br> where |A| = max(|Align|(|T1|,`uniform`),..., |Align|(|Tn|,`uniform`)))
<td>struct&lt;T<sub>0</sub>, ..., T<sub>N</sub>&gt;
<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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it boils down to 'roundup(k, ...)' distributes over max, which I believe is true .

wgsl/index.bs Show resolved Hide resolved
@kainino0x kainino0x added the wgsl WebGPU Shading Language Issues label Mar 8, 2021
@kdashg
Copy link
Contributor

kdashg commented Mar 9, 2021

Merge when ready!

@dneto0
Copy link
Contributor

dneto0 commented Mar 9, 2021

I think remaining things are editorial:

  • clarify and/or constructions.
  • Editorial: I think we need to define terms "effective stride", "effective size", "effective alignment" that takes into account defaulting vs. explicit attributes.
  • Maybe simplify roundup over max of structure member alignments.
  • Gather all rules for alignment into one place, with cases for storage vs. uniform storage class

Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've logged the editorial items to follow up.

@dneto0 dneto0 merged commit 734688e into gpuweb:main Mar 9, 2021
WGSL automation moved this from Needs Approval to Done Mar 9, 2021
@ben-clayton ben-clayton deleted the default-struct-layout branch March 9, 2021 20:59
@kainino0x kainino0x linked an issue Mar 9, 2021 that may be closed by this pull request
ben-clayton added a commit to ben-clayton/Tint that referenced this pull request Apr 13, 2021
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 <bclayton@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wgsl WebGPU Shading Language Issues
Projects
WGSL
Done
Development

Successfully merging this pull request may close these issues.

[WGSL] Proposal: Default Struct Layout
7 participants