Skip to content

Commit

Permalink
[d3d12] bound check dynamic buffers
Browse files Browse the repository at this point in the history
To achieve this for dynamic storage buffers we changed the way we bind them. They now go in root tables and we pass the offset via a root constant.
  • Loading branch information
teoxoy committed Jan 22, 2025
1 parent 3dd925b commit f8089d9
Show file tree
Hide file tree
Showing 11 changed files with 525 additions and 96 deletions.
2 changes: 2 additions & 0 deletions naga/src/back/hlsl/help.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1248,6 +1248,8 @@ impl<W: Write> super::Writer<'_, W> {
space: u8::MAX,
register: key.group,
binding_array_size: None,
dynamic_storage_buffer_offsets_index: None,
restrict_indexing: false,
},
None => {
unreachable!("Sampler buffer of group {key:?} not bound to a register");
Expand Down
2 changes: 2 additions & 0 deletions naga/src/back/hlsl/keywords.rs
Original file line number Diff line number Diff line change
Expand Up @@ -907,3 +907,5 @@ pub const TYPES: &[&str] = &{

res
};

pub const RESERVED_PREFIXES: &[&str] = &["__dynamic_buffer_offsets"];
26 changes: 26 additions & 0 deletions naga/src/back/hlsl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,23 @@ pub struct BindTarget {
pub register: u32,
/// If the binding is an unsized binding array, this overrides the size.
pub binding_array_size: Option<u32>,
/// This is the index in the buffer at [`Options::dynamic_storage_buffer_offsets_targets`].
pub dynamic_storage_buffer_offsets_index: Option<u32>,
/// This is a hint that we need to restrict indexing of vectors, matrices and arrays.
///
/// If [`Options::restrict_indexing`] is also `true`, we will restrict indexing.
#[cfg_attr(any(feature = "serialize", feature = "deserialize"), serde(default))]
pub restrict_indexing: bool,
}

#[derive(Clone, Debug, Default, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "serialize", derive(serde::Serialize))]
#[cfg_attr(feature = "deserialize", derive(serde::Deserialize))]
/// BindTarget for dynamic storage buffer offsets
pub struct OffsetsBindTarget {
pub space: u8,
pub register: u32,
pub size: u32,
}

// Using `BTreeMap` instead of `HashMap` so that we can hash itself.
Expand Down Expand Up @@ -214,11 +231,15 @@ impl Default for SamplerHeapBindTargets {
space: 0,
register: 0,
binding_array_size: None,
dynamic_storage_buffer_offsets_index: None,
restrict_indexing: false,
},
comparison_samplers: BindTarget {
space: 1,
register: 0,
binding_array_size: None,
dynamic_storage_buffer_offsets_index: None,
restrict_indexing: false,
},
}
}
Expand Down Expand Up @@ -260,6 +281,8 @@ pub struct Options {
pub sampler_heap_target: SamplerHeapBindTargets,
/// Mapping of each bind group's sampler index buffer to a bind target.
pub sampler_buffer_binding_map: SamplerIndexBufferBindingMap,
/// Bind target for dynamic storage buffer offsets
pub dynamic_storage_buffer_offsets_targets: std::collections::BTreeMap<u32, OffsetsBindTarget>,
/// Should workgroup variables be zero initialized (by polyfilling)?
pub zero_initialize_workgroup_memory: bool,
/// Should we restrict indexing of vectors, matrices and arrays?
Expand All @@ -276,6 +299,7 @@ impl Default for Options {
sampler_heap_target: SamplerHeapBindTargets::default(),
sampler_buffer_binding_map: std::collections::BTreeMap::default(),
push_constants_target: None,
dynamic_storage_buffer_offsets_targets: std::collections::BTreeMap::new(),
zero_initialize_workgroup_memory: true,
restrict_indexing: true,
}
Expand All @@ -293,6 +317,8 @@ impl Options {
space: res_binding.group as u8,
register: res_binding.binding,
binding_array_size: None,
dynamic_storage_buffer_offsets_index: None,
restrict_indexing: false,
}),
None => Err(EntryPointError::MissingBinding(*res_binding)),
}
Expand Down
24 changes: 23 additions & 1 deletion naga/src/back/hlsl/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ const STORE_TEMP_NAME: &str = "_value";
/// [`Storage`]: crate::AddressSpace::Storage
#[derive(Debug)]
pub(super) enum SubAccess {
BufferOffset {
group: u32,
offset: u32,
},

/// Add the given byte offset. This is used for struct members, or
/// known components of a vector or matrix. In all those cases,
/// the byte offset is a compile-time constant.
Expand Down Expand Up @@ -119,6 +124,9 @@ impl<W: fmt::Write> super::Writer<'_, W> {
write!(self.out, "+")?;
}
match *access {
SubAccess::BufferOffset { group, offset } => {
write!(self.out, "__dynamic_buffer_offsets{group}._{offset}")?;
}
SubAccess::Offset(offset) => {
write!(self.out, "{offset}")?;
}
Expand Down Expand Up @@ -492,7 +500,21 @@ impl<W: fmt::Write> super::Writer<'_, W> {

loop {
let (next_expr, access_index) = match func_ctx.expressions[cur_expr] {
crate::Expression::GlobalVariable(handle) => return Ok(handle),
crate::Expression::GlobalVariable(handle) => {
if let Some(ref binding) = module.global_variables[handle].binding {
// this was already resolved earlier when we started evaluating an entry point.
let bt = self.options.resolve_resource_binding(binding).unwrap();
if let Some(dynamic_storage_buffer_offsets_index) =
bt.dynamic_storage_buffer_offsets_index
{
self.temp_access_chain.push(SubAccess::BufferOffset {
group: binding.group,
offset: dynamic_storage_buffer_offsets_index,
});
}
}
return Ok(handle);
}
crate::Expression::Access { base, index } => (base, AccessIndex::Expression(index)),
crate::Expression::AccessIndex { base, index } => {
(base, AccessIndex::Constant(index))
Expand Down
33 changes: 31 additions & 2 deletions naga/src/back/hlsl/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
super::keywords::RESERVED,
super::keywords::TYPES,
super::keywords::RESERVED_CASE_INSENSITIVE,
&[],
super::keywords::RESERVED_PREFIXES,
&mut self.names,
);
self.entry_point_io.clear();
Expand Down Expand Up @@ -256,6 +256,22 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
writeln!(self.out)?;
}

for (group, bt) in self.options.dynamic_storage_buffer_offsets_targets.iter() {
writeln!(self.out, "struct __dynamic_buffer_offsetsTy{} {{", group)?;
for i in 0..bt.size {
writeln!(self.out, "{}uint _{};", back::INDENT, i)?;
}
writeln!(self.out, "}};")?;
writeln!(
self.out,
"ConstantBuffer<__dynamic_buffer_offsetsTy{}> __dynamic_buffer_offsets{}: register(b{}, space{});",
group, group, bt.register, bt.space
)?;

// Extra newline for readability
writeln!(self.out)?;
}

// Save all entry point output types
let ep_results = module
.entry_points
Expand Down Expand Up @@ -2777,7 +2793,20 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
| crate::AddressSpace::PushConstant,
)
| None => true,
Some(crate::AddressSpace::Uniform) => false, // TODO: needs checks for dynamic uniform buffers, see https://github.com/gfx-rs/wgpu/issues/4483
Some(crate::AddressSpace::Uniform) => {
// check if BindTarget.restrict_indexing is set, this is used for dynamic buffers
let var_handle = self.fill_access_chain(module, base, func_ctx)?;
let bind_target = self
.options
.resolve_resource_binding(
module.global_variables[var_handle]
.binding
.as_ref()
.unwrap(),
)
.unwrap();
bind_target.restrict_indexing
}
Some(
crate::AddressSpace::Handle | crate::AddressSpace::Storage { .. },
) => unreachable!(),
Expand Down
9 changes: 7 additions & 2 deletions naga/tests/snapshots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,13 @@ impl Input {
let mut param_path = self.input_path();
param_path.set_extension("param.ron");
match fs::read_to_string(&param_path) {
Ok(string) => ron::de::from_str(&string)
.unwrap_or_else(|_| panic!("Couldn't parse param file: {}", param_path.display())),
Ok(string) => match ron::de::from_str(&string) {
Ok(params) => params,
Err(e) => panic!(
"Couldn't parse param file: {} due to: {e}",
param_path.display()
),
},
Err(_) => Parameters::default(),
}
}
Expand Down
Loading

0 comments on commit f8089d9

Please sign in to comment.