vulkano-rs/vulkano

`command_builder.push_constants` won't push data if `push_constant_ranges` is not aligned

TrevorCow opened this issue · 9 comments

Build Info

  • Version of vulkano: main
  • OS: Windows
  • GPU (the selected PhysicalDevice): RTX 3080 ti
  • GPU Driver: nVidia 545.84

Issue

I'm not super knowledgeable on alignment so please tell me if I am misunderstanding something, thank you!

When defining a push_constant_range in PipelineLayoutCreateInfo if you have something like

PipelineLayout::new(
    ...
    PipelineLayoutCreateInfo {
        push_constant_ranges: vec![
            PushConstantRange {
                stages: ShaderStages::VERTEX | ShaderStages::FRAGMENT,
                offset: 0,
                size: (size_of::<Mat4>() + (size_of::<u32>() * 2)) as u32,
            },
        ],
        ...
}).unwrap();

This defines a push constant with a "useable size" of 72 (Mat4 = 64 + 2 * (u32 = 4)), however, because of GPU alignment, the real push constant size is 80.

So, when trying to push data

#[derive(BufferContents)]
#[repr(C)]
struct PushConstants {
    transform: Mat4,
    index1: u32,
    index2: u32,
}
let pc: PushConstnats = ...;

command_builder.push_constants(graphics_pipeline.layout().clone(), 0, pc).unwrap();

panics (technically returns Err but either way the command fails) with one or more bytes of `push_constants` are not within any push constant range of `pipeline_layout`. Changing the push constant range to size: (size_of::<Mat4>() + (size_of::<u32>() * 4)) as u32 the code works as expected. The above code snippet should work since the requested size matches the push size.

Rua commented

What is the size of the PushConstants struct?

interestingly... 80. I'm guessing the BufferContents derive automatically pads it? It appears it does:

unsafe impl ::vulkano::buffer::BufferContents for GLTFPushConstants {
    const LAYOUT: ::vulkano::buffer::BufferContentsLayout = {
        {
            #[allow(unused)]
            fn bound() {
                fn assert_impl<T: ::vulkano::buffer::BufferContents + ?Sized>() {}
                assert_impl::<Mat4>();
            }
        }
        {
            #[allow(unused)]
            fn bound() {
                fn assert_impl<T: ::vulkano::buffer::BufferContents + ?Sized>() {}
                assert_impl::<u32>();
            }
        }
        {
            #[allow(unused)]
            fn bound() {
                fn assert_impl<T: ::vulkano::buffer::BufferContents + ?Sized>() {}
                assert_impl::<u32>();
            }
        } const fn extend_layout(layout: ::std::alloc::Layout, next: ::std::alloc::Layout) -> ::std::alloc::Layout {
            let padded_size = if let Some(val) = layout.size().checked_add(next.align() - 1) { val & !(next.align() - 1) } else { ::std::unreachable!() };
            let align = if layout.align() >= next.align() { layout.align() } else { next.align() };
            if let Some(size) = padded_size.checked_add(next.size()) { if let Ok(layout) = ::std::alloc::Layout::from_size_align(size, align) { layout } else { ::std::unreachable!() } } else { ::std::unreachable!() }
        }
        if let Some(layout) = <u32 as ::vulkano::buffer::BufferContents>::LAYOUT.extend_from_layout(&extend_layout(extend_layout(::std::alloc::Layout::new::<()>(), ::std::alloc::Layout::new::<Mat4>()), ::std::alloc::Layout::new::<u32>())) { if let Some(layout) = layout.pad_to_alignment() { layout } else { ::std::unreachable!() } } else { ::std::panic!("zero-sized types are not valid buffer contents") }
    };
...
Rua commented

Yeah it seems so. But it must have something to do with your Mat4 type; if I replicate it with a [[f32; 4]; 4] as replacement, the size is 72.

Woah yes very interesting, replacing Mat4 with [[f32; 4]; 4] has a size of 72, and the rest of the code works. However, size_of::<Mat4>: 64, size_of::<[[f32; 4]; 4]>: 64. So is this a problem with the #[derive(BufferContents)] then?

Note: The Mat4 type is from the glam crate. Defined

#[derive(Clone, Copy)]
#[repr(C)]
pub struct Mat4 {
    pub x_axis: Vec4,
    pub y_axis: Vec4,
    pub z_axis: Vec4,
    pub w_axis: Vec4,
}

With Vec4 defined

/// A 4-dimensional vector.
///
/// SIMD vector types are used for storage on supported platforms.
///
/// This type is 16 byte aligned.
#[derive(Clone, Copy)]
#[repr(transparent)]
pub struct Vec4(pub(crate) __m128);
Rua commented

The problem seems to be with alignment rather than size, which you can check with std::mem::align_of. The documentation states that Vec4 is 16-byte aligned, and so when you define a struct with a Vec4 in it, it has to align that struct to 16 bytes too. Unfortunately, Rust can't distinguish size from alignment in this case, so it forces the size to be 80 as well.

Right I just checked that

size_of::<Mat4>: 64
align_of::<Mat4>: 16
size_of::<[[f32; 4]; 4]>: 64
align_of::<[[f32; 4]; 4]>: 4
size_of::<PushConstantsMat4>: 80 
align_of::<PushConstantsMat4>: 16 
size_of::<PushConstantsFloats>: 72 
align_of::<PushConstantsFloats>: 4 

Ok so is this a non-problem/won't fix? Seems to be just something that has to happen when using a SIMD Vec4 because of its larger 16-byte alignment.

Rua commented

Yeah, there isn't really any way that Vulkano can tell Rust to make the size 72. The size always has to be a multiple of the alignment.

Yeah, I think the only way to implement it would be some way of recursively calling size_of on the members of a struct until its only primitives and then adding them up. I don't know if this would cause any other problems or if that would even be possible