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

Add Single Member Construction Functions #46

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Alloc on offset change
cwfitzgerald committed Apr 16, 2024
commit 8cbd2c7fd8b502b78b647b831b2b749e48ce3ff3
42 changes: 27 additions & 15 deletions src/core/buffers.rs
Original file line number Diff line number Diff line change
@@ -195,6 +195,17 @@ impl<B> AsMut<B> for DynamicStorageBuffer<B> {
}

impl<B: BufferMut> DynamicStorageBuffer<B> {
fn set_and_alloc_offset(&mut self, offset: usize) -> Result<()> {
self.offset = offset;

self.inner
.try_enlarge(self.offset)
.map_err(|_e| super::rw::Error::BufferTooSmall {
expected: self.offset as u64,
found: self.inner.capacity() as u64,
})
}

/// Layouts and writes an entire bound value into the buffer. The value is written at the
/// next available offset after the buffer's current offset, which is aligned to the required
/// dynamic binding alignment (defaults to 256).
@@ -206,13 +217,13 @@ impl<B: BufferMut> DynamicStorageBuffer<B> {
where
T: ?Sized + ShaderType + WriteInto,
{
self.write_dynamic_struct_break();
self.write_struct_end()?;
let offset = self.offset;

let mut writer = Writer::new(value, &mut self.inner, offset)?;
value.write_into(&mut writer);

self.offset += value.size().get() as usize;
self.set_and_alloc_offset(self.offset + value.size().get() as usize)?;

Ok(offset as u64)
}
@@ -222,11 +233,11 @@ impl<B: BufferMut> DynamicStorageBuffer<B> {
/// alignment of `T`.
///
/// The use case is constructing a struct member by member in case the layout isn't
/// known at compile time. Combine this with [`Self::write_dynamic_struct_break`] to "end"
/// known at compile time. Combine this with [`Self::write_struct_end`] to "end"
/// the struct being written and start the next one.
///
/// Returns the offset at which the value was written.
pub fn write_single_member<T>(&mut self, value: &T) -> Result<u64>
pub fn write_struct_member<T>(&mut self, value: &T) -> Result<u64>
where
T: ShaderType + WriteInto,
{
@@ -236,7 +247,7 @@ impl<B: BufferMut> DynamicStorageBuffer<B> {
let mut writer = Writer::new(value, &mut self.inner, offset)?;
value.write_into(&mut writer);

self.offset += value.size().get() as usize;
self.set_and_alloc_offset(self.offset + value.size().get() as usize)?;

Ok(offset as u64)
}
@@ -245,13 +256,14 @@ impl<B: BufferMut> DynamicStorageBuffer<B> {
/// to the required dynamic binding alignment (defaults to 256).
///
/// The use case is constructing a struct member by member in case the layout isn't
/// known at compile time. Combine this with [`Self::write_single_member`] to add
/// known at compile time. Combine this with [`Self::write_struct_member`] to add
/// each individual member of the struct being written.
///
/// Returns the offset which was rounded up to.
pub fn write_dynamic_struct_break(&mut self) -> u64 {
self.offset = self.alignment.round_up(self.offset as u64) as usize;
self.offset as u64
pub fn write_struct_end(&mut self) -> Result<u64> {
self.set_and_alloc_offset(self.alignment.round_up(self.offset as u64) as usize)?;

Ok(self.offset as u64)
}
}

@@ -394,28 +406,28 @@ impl<B: BufferMut> DynamicUniformBuffer<B> {
/// alignment of `T`.
///
/// The use case is constructing a struct member by member in case the layout isn't
/// known at compile time. Combine this with [`Self::write_dynamic_struct_break`] to "end"
/// known at compile time. Combine this with [`Self::write_struct_end`] to "end"
/// the struct being written and start the next one.
///
/// Returns the offset at which the value was written.
pub fn write_single_member<T>(&mut self, value: &T) -> Result<u64>
pub fn write_struct_member<T>(&mut self, value: &T) -> Result<u64>
where
T: ShaderType + WriteInto,
{
T::assert_uniform_compat();
Copy link
Owner

Choose a reason for hiding this comment

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

This assert won't suffice, since there are 2 more invariants that need to be checked.

let field_offset_check = quote_spanned! {ident.span()=>
if let ::core::option::Option::Some(min_alignment) =
<#ty as #root::ShaderType>::METADATA.uniform_min_alignment()
{
let offset = <Self as #root::ShaderType>::METADATA.offset(#i);
#root::concat_assert!(
min_alignment.is_aligned(offset),
"offset of field '", #name, "' must be a multiple of ", min_alignment.get(),
" (current offset: ", offset, ")"
)
}
};
let field_offset_diff = if i != 0 {
let prev_field = &field_data[i - 1];
let prev_field_ty = &prev_field.field.ty;
let prev_ident_name = prev_field.ident().to_string();
quote_spanned! {ident.span()=>
if let ::core::option::Option::Some(min_alignment) =
<#prev_field_ty as #root::ShaderType>::METADATA.uniform_min_alignment()
{
let prev_offset = <Self as #root::ShaderType>::METADATA.offset(#i - 1);
let offset = <Self as #root::ShaderType>::METADATA.offset(#i);
let diff = offset - prev_offset;
let prev_size = <#prev_field_ty as #root::ShaderSize>::SHADER_SIZE.get();
let prev_size = min_alignment.round_up(prev_size);
#root::concat_assert!(
diff >= prev_size,
"offset between fields '", #prev_ident_name, "' and '", #name, "' must be at least ",
min_alignment.get(), " (currently: ", diff, ")"
)
}
}

Copy link
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 what exact rules these are checking for. Could you either explain them or point to the wgsl spec for them.

Copy link
Owner

Choose a reason for hiding this comment

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

They are listed in https://gpuweb.github.io/gpuweb/wgsl/#address-space-layout-constraints but the short version is that:

  • the offset of array/struct members needs to be aligned to 16
  • given a member of type struct (S) at offset o0 and its subsequent member at offset o1,
    o1 - o0 >= roundUp(16, SizeOf(S)) must be true (definitions: roundUp SizeOf)

self.inner.write_single_member(value)
self.inner.write_struct_member(value)
}

/// Writes a "struct break" into the buffer. This takes the buffer offset and aligns it
/// to the required dynamic binding alignment (defaults to 256).
///
/// The use case is constructing a struct member by member in case the layout isn't
/// known at compile time. Combine this with [`Self::write_single_member`] to add
/// known at compile time. Combine this with [`Self::write_struct_member`] to add
/// each individual member of the struct being written.
///
/// Returns the offset which was rounded up to.
pub fn write_dynamic_struct_break(&mut self) -> u64 {
self.inner.write_dynamic_struct_break()
pub fn write_struct_end(&mut self) -> Result<u64> {
self.inner.write_struct_end()
}
}