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

Incorrect array stride for arrays of vector types #573

Open
msiglreith opened this issue Mar 31, 2021 · 4 comments
Open

Incorrect array stride for arrays of vector types #573

msiglreith opened this issue Mar 31, 2021 · 4 comments
Labels
c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) t: bug Something isn't working

Comments

@msiglreith
Copy link
Contributor

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 msiglreith added the t: bug Something isn't working label Mar 31, 2021
@msiglreith msiglreith changed the title Incorrect array stride for vector types Incorrect array stride for arrays of vector types Mar 31, 2021
@msiglreith
Copy link
Contributor Author

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..

@hrydgard
Copy link
Contributor

hrydgard commented Apr 1, 2021

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..

@XAMPPRocky XAMPPRocky added the c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. label Apr 1, 2021
@eddyb
Copy link
Contributor

eddyb commented Mar 29, 2023

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?

@oisyn
Copy link
Contributor

oisyn commented Mar 29, 2023

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.

@eddyb eddyb added the s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) t: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants
@eddyb @hrydgard @XAMPPRocky @msiglreith @oisyn and others