From d2f9f0c134e0b0be9e604a34ef9840ad00141f94 Mon Sep 17 00:00:00 2001 From: Amjad Alsharafi Date: Wed, 9 Feb 2022 20:10:13 +0800 Subject: [PATCH] Added `overlapping_push_constant_ranges` to `PipelineLayout` (#1820) --- vulkano/src/command_buffer/auto.rs | 81 ++++---- vulkano/src/pipeline/layout.rs | 287 ++++++++++++++++++++++++++++- 2 files changed, 325 insertions(+), 43 deletions(-) diff --git a/vulkano/src/command_buffer/auto.rs b/vulkano/src/command_buffer/auto.rs index 0d291e9954..8350a59ed4 100644 --- a/vulkano/src/command_buffer/auto.rs +++ b/vulkano/src/command_buffer/auto.rs @@ -79,7 +79,6 @@ use crate::render_pass::Framebuffer; use crate::render_pass::LoadOp; use crate::render_pass::Subpass; use crate::sampler::Filter; -use crate::shader::ShaderStages; use crate::sync::AccessCheckError; use crate::sync::AccessFlags; use crate::sync::GpuFuture; @@ -1840,51 +1839,51 @@ impl AutoCommandBufferBuilder { "the size of push_constants must be a multiple of 4" ); - // Figure out which shader stages in the pipeline layout overlap this byte range. - // Also check that none of the bytes being set are outside all push constant ranges. - let shader_stages = pipeline_layout - .push_constant_ranges() + // SAFETY: `&push_constants` is a valid pointer, and the size of the struct is `size`, + // thus, getting a slice of the whole struct is safe if its not modified. + let whole_data = unsafe { + slice::from_raw_parts(&push_constants as *const Pc as *const u8, size as usize) + }; + + let mut current_offset = offset; + let mut remaining_size = size; + for range in pipeline_layout + .push_constant_ranges_disjoint() .iter() - .filter(|range| range.offset < offset + size && offset < range.offset + range.size) - .try_fold( - (ShaderStages::none(), offset), - |(shader_stages, last_bound), range| { - if range.offset > last_bound { - Err(()) - } else { - Ok(( - shader_stages.union(&range.stages), - last_bound.max(range.offset + range.size), - )) - } - }, - ) - .and_then(|(shader_stages, last_bound)| { - if shader_stages == ShaderStages::none() || last_bound < offset + size { - Err(()) - } else { - Ok(shader_stages) - } - }) - .expect( - "not all bytes in push_constants fall within the pipeline layout's push constant ranges", - ); + .skip_while(|range| range.offset + range.size <= offset) + { + // there is a gap between ranges, but the passed push_constants contains + // some bytes in this gap, exit the loop and report error + if range.offset > current_offset { + break; + } - unsafe { - let data = slice::from_raw_parts( - (&push_constants as *const Pc as *const u8).offset(offset as isize), - size as usize, - ); + // push the minimum of the whole remaining data, and the part until the end of this range + let push_size = remaining_size.min(range.offset + range.size - current_offset); + let data_offset = (current_offset - offset) as usize; + unsafe { + self.inner.push_constants::<[u8]>( + pipeline_layout.clone(), + range.stages, + current_offset, + push_size, + &whole_data[data_offset..(data_offset + push_size as usize)], + ); + } + current_offset += push_size; + remaining_size -= push_size; - self.inner.push_constants::<[u8]>( - pipeline_layout.clone(), - shader_stages, - offset, - size, - data, - ); + if remaining_size == 0 { + break; + } } + assert!( + remaining_size == 0, + "There exists data at offset {} that is not included in any range", + current_offset, + ); + self } diff --git a/vulkano/src/pipeline/layout.rs b/vulkano/src/pipeline/layout.rs index 21dad3edcc..24e2021fdb 100644 --- a/vulkano/src/pipeline/layout.rs +++ b/vulkano/src/pipeline/layout.rs @@ -94,7 +94,8 @@ pub struct PipelineLayout { handle: ash::vk::PipelineLayout, device: Arc, descriptor_set_layouts: SmallVec<[Arc; 4]>, - push_constant_ranges: SmallVec<[PipelineLayoutPcRange; 4]>, + push_constant_ranges: SmallVec<[PipelineLayoutPcRange; 5]>, + push_constant_ranges_disjoint: SmallVec<[PipelineLayoutPcRange; 5]>, } impl PipelineLayout { @@ -122,7 +123,7 @@ impl PipelineLayout { return Err(PipelineLayoutCreationError::MultiplePushDescriptor); } - let mut push_constant_ranges: SmallVec<[PipelineLayoutPcRange; 4]> = + let mut push_constant_ranges: SmallVec<[PipelineLayoutPcRange; 5]> = push_constant_ranges.into_iter().collect(); // Check for overlapping stages @@ -148,6 +149,42 @@ impl PipelineLayout { ) }); + let mut push_constant_ranges_disjoint: SmallVec<[PipelineLayoutPcRange; 5]> = + SmallVec::new(); + + if !push_constant_ranges.is_empty() { + let mut min_offset = push_constant_ranges[0].offset; + loop { + let mut max_offset = u32::MAX; + let mut stages = ShaderStages::none(); + + for range in &push_constant_ranges { + // new start (begin next time from it) + if range.offset > min_offset { + max_offset = max_offset.min(range.offset); + break; + } else if range.offset + range.size > min_offset { + // inside the range, include the stage + // use the minimum of the end of all ranges that are overlapping + max_offset = max_offset.min(range.offset + range.size); + stages = stages | range.stages; + } + } + // finished all stages + if stages == ShaderStages::none() { + break; + } + + push_constant_ranges_disjoint.push(PipelineLayoutPcRange { + offset: min_offset, + size: max_offset - min_offset, + stages, + }); + // prepare for next range + min_offset = max_offset; + } + } + // Check against device limits check_desc_against_limits( device.physical_device().properties(), @@ -230,6 +267,7 @@ impl PipelineLayout { device: device.clone(), descriptor_set_layouts, push_constant_ranges, + push_constant_ranges_disjoint, })) } @@ -248,6 +286,23 @@ impl PipelineLayout { &self.push_constant_ranges } + /// Returns a slice containing the push constant ranges in with all disjoint stages. + /// + /// For example, if we have these `push_constant_ranges`: + /// - `offset=0, size=4, stages=vertex` + /// - `offset=0, size=12, stages=fragment` + /// + /// The returned value will be: + /// - `offset=0, size=4, stages=vertex|fragment` + /// - `offset=4, size=8, stages=fragment` + /// + /// The ranges are guaranteed to be sorted deterministically by offset, and + /// guaranteed to be disjoint, meaning that there is no overlap between the ranges. + #[inline] + pub(crate) fn push_constant_ranges_disjoint(&self) -> &[PipelineLayoutPcRange] { + &self.push_constant_ranges_disjoint + } + /// Returns whether `self` is compatible with `other` for the given number of sets. pub fn is_compatible_with(&self, other: &PipelineLayout, num_sets: u32) -> bool { let num_sets = num_sets as usize; @@ -1023,6 +1078,234 @@ impl Counter { } } +#[cfg(test)] +mod tests { + + use crate::{pipeline::layout::PipelineLayoutPcRange, shader::ShaderStages}; + + use super::PipelineLayout; + + #[test] + fn push_constant_ranges_disjoint() { + let test_cases = [ + // input: + // - `0..12`, stage=fragment + // - `0..40`, stage=vertex + // + // output: + // - `0..12`, stage=fragment|vertex + // - `12..40`, stage=vertex + ( + &[ + PipelineLayoutPcRange { + offset: 0, + size: 12, + stages: ShaderStages { + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 0, + size: 40, + stages: ShaderStages { + vertex: true, + ..Default::default() + }, + }, + ][..], + &[ + PipelineLayoutPcRange { + offset: 0, + size: 12, + stages: ShaderStages { + vertex: true, + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 12, + size: 28, + stages: ShaderStages { + vertex: true, + ..Default::default() + }, + }, + ][..], + ), + // input: + // - `0..12`, stage=fragment + // - `4..40`, stage=vertex + // + // output: + // - `0..4`, stage=fragment + // - `4..12`, stage=fragment|vertex + // - `12..40`, stage=vertex + ( + &[ + PipelineLayoutPcRange { + offset: 0, + size: 12, + stages: ShaderStages { + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 4, + size: 36, + stages: ShaderStages { + vertex: true, + ..Default::default() + }, + }, + ][..], + &[ + PipelineLayoutPcRange { + offset: 0, + size: 4, + stages: ShaderStages { + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 4, + size: 8, + stages: ShaderStages { + fragment: true, + vertex: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 12, + size: 28, + stages: ShaderStages { + vertex: true, + ..Default::default() + }, + }, + ][..], + ), + // input: + // - `0..12`, stage=fragment + // - `8..20`, stage=compute + // - `4..16`, stage=vertex + // - `8..32`, stage=tess_ctl + // + // output: + // - `0..4`, stage=fragment + // - `4..8`, stage=fragment|vertex + // - `8..16`, stage=fragment|vertex|compute|tess_ctl + // - `16..20`, stage=compute|tess_ctl + // - `20..32` stage=tess_ctl + ( + &[ + PipelineLayoutPcRange { + offset: 0, + size: 12, + stages: ShaderStages { + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 8, + size: 12, + stages: ShaderStages { + compute: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 4, + size: 12, + stages: ShaderStages { + vertex: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 8, + size: 24, + stages: ShaderStages { + tessellation_control: true, + ..Default::default() + }, + }, + ][..], + &[ + PipelineLayoutPcRange { + offset: 0, + size: 4, + stages: ShaderStages { + fragment: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 4, + size: 4, + stages: ShaderStages { + fragment: true, + vertex: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 8, + size: 4, + stages: ShaderStages { + vertex: true, + fragment: true, + compute: true, + tessellation_control: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 12, + size: 4, + stages: ShaderStages { + vertex: true, + compute: true, + tessellation_control: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 16, + size: 4, + stages: ShaderStages { + compute: true, + tessellation_control: true, + ..Default::default() + }, + }, + PipelineLayoutPcRange { + offset: 20, + size: 12, + stages: ShaderStages { + tessellation_control: true, + ..Default::default() + }, + }, + ][..], + ), + ]; + + let (device, _) = gfx_dev_and_queue!(); + + for (input, expected) in test_cases { + let layout = PipelineLayout::new(device.clone(), [], input.iter().cloned()).unwrap(); + + assert_eq!(layout.push_constant_ranges_disjoint.as_slice(), expected); + } + } +} + /* TODO: restore #[cfg(test)] mod tests {