rust-gpu icon indicating copy to clipboard operation
rust-gpu copied to clipboard

Incorrect array stride for arrays of vector types

Open msiglreith opened this issue 4 years ago • 4 comments

const QUAD_CLIP: [f32x2; 4] = [
    f32x2 { x: -1.0, y: -1.0 },
    f32x2 { x: 1.0, y: -1.0 },
    f32x2 { x: -1.0, y: 1.0 },
    f32x2 { x: 1.0, y: 1.0 },
];

#[spirv(vertex)]
pub fn quad_vs(#[spirv(vertex_id)] vert_id: i32, #[spirv(position)] a_position: &mut f32x4) {
    let idx = vert_id as usize;
    let pos_clip = QUAD_CLIP[idx];
    *a_position = vec4(pos_clip.x, pos_clip.y, idx as f32, 1.0);
}

QUAD_CLIP type will be generated with an ArrayStride of 8 which is incorrect as in this case all array elements should be treated as vec4 types, requiring a stride of 16.

Generated:

OpDecorate %_arr_v2float_uint_4 ArrayStride 8

Expected:

OpDecorate %_arr_v2float_uint_4 ArrayStride 16

msiglreith avatar Mar 31 '21 18:03 msiglreith

maybe the decoration should be even removed because I don't recall if the stride is defined at all if not part of the shader resource interface..

msiglreith avatar Mar 31 '21 22:03 msiglreith

Right - in a uniform buffer with no layout extensions, and thus std140 layout, the stride should be 16, and in a storage buffer with 430 layout, 8. For a plain array like that, it seems kinda arbitrary..

hrydgard avatar Apr 01 '21 06:04 hrydgard

in a uniform buffer with no layout extensions

QUAD_CLIP is not an UBO here, so I'm confused why the mention.

QUAD_CLIP type will be generated with an ArrayStride of 8 which is incorrect as in this case all array elements should be treated as vec4 types, requiring a stride of 16.

Why should they be treated as such? The only rules I know of that have those requirements are for UBOs/SSBOs, and even there it's an archaic mess that I should we should paper over (e.g. if you had that [f32x2; 4] in an UBO, long-term we should probably turn that into a [f32x4; 2] for you, to maximize compatibility with the scalar layout which is the only thing you can easily automate on the CPU side).

Are you getting an error from anything? Like a validation error that prompted this?

EDIT: according to the Vulkan spec:

If the uniformBufferStandardLayout feature is not enabled on the device, then any member of an OpTypeStruct with a storage class of Uniform and a decoration of Block must be aligned according to its extended alignment.

Unsure which combination opts into that on the spirv-val side but I suspect SpirvBuilder::uniform_buffer_standard_layout might do it?

eddyb avatar Mar 29 '23 12:03 eddyb

For sake of completeness, I altered your example somewhat to use glam and appropriate syntax:

#[spirv(vertex)]
pub fn quad_vs(
    #[spirv(vertex_index)] vert_id: i32,
    #[spirv(uniform, descriptor_set = 0, binding = 0)] clip: &[Vec2; 4],
    #[spirv(position)] a_position: &mut Vec4,
) {
    let idx = vert_id as usize;
    let pos_clip = clip[idx];
    *a_position = Vec4::new(pos_clip.x, pos_clip.y, idx as f32, 1.0);
}

And I get this error

  error: Structure id 10 decorated as Block for variable in Uniform storage class must follow relaxed uniform buffer layout rules: member 0 contains an array with stride 8 not satisfying alignment to 16
           %_struct_10 = OpTypeStruct %_arr_v2float_uint_4

So at least it throws an error when using an incorrect alignment for uniform buffers.

oisyn avatar Mar 29 '23 13:03 oisyn