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

Make #[spirv(block)] decoration force ABI type into Struct #391

Closed
fu5ha opened this issue Jan 22, 2021 · 6 comments · Fixed by #576
Closed

Make #[spirv(block)] decoration force ABI type into Struct #391

fu5ha opened this issue Jan 22, 2021 · 6 comments · Fixed by #576
Assignees
Labels
a: ark Issues that Ark depends on t: enhancement A new feature or improvement to an existing one.

Comments

@fu5ha
Copy link
Member

fu5ha commented Jan 22, 2021

In a situation where one wants to use a non-struct primitive type as a push constant or uniform, you need to put it into a struct and apply the block decoration to it. However, if we try to just put it in a wrapper struct the ABI will not actually change. For example this

#[spirv(block)]
#[derive(Clone, Copy)]
#[allow(unused_attributes)]
pub struct Constants {
    direction: Vec2,
    _make_struct: f32,
}

without the _make_struct field, this will still be an AbiVector and so it's illegal to apply the block decoration to it. This also occurs for two f32s in which case it will be a ScalarPair, like here:

#[spirv(block)]
#[derive(Copy, Clone)]
#[allow(unused_attributes)]
pub struct TonemapConstants {
    pub exposure: f32,
    pub aspect: f32,
    _make_struct: f32,
}

It would be nice to not have to add this extra member to the struct.

@fu5ha fu5ha added t: enhancement A new feature or improvement to an existing one. a: ark Issues that Ark depends on labels Jan 22, 2021
@khyperia
Copy link
Contributor

Just checking, does adding #[repr(C)] fix the issue?

@fu5ha
Copy link
Member Author

fu5ha commented Jan 22, 2021

Evidently not, doesn't compile
image

@fu5ha
Copy link
Member Author

fu5ha commented Jan 22, 2021

The other case also doesn't work
image

@khyperia
Copy link
Contributor

Right, the first issue is #373, but I'm really confused by the second issue, that's really bizarre and like #[repr(C)] didn't actually apply (or I don't understand #[repr(C)])

@eddyb eddyb self-assigned this Apr 4, 2021
@eddyb
Copy link
Contributor

eddyb commented Apr 4, 2021

We can fix this with a query override, and I've been meaning to try it, but would it be better to just not need #[spirv(block)], and wrap every uniform automatically in an OpTypeStruct with a Block decoration?

I can't find anywhere in SPIR-V or Vulkan specs why extra layers of single-field structs would change anything - there's some stuff about Block-decorated OpTypeStruct having Offset decorations on their members, but I don't really see that resulting in different values than a single struct-typed field with an Offset of 0.

@hrydgard
Copy link
Contributor

hrydgard commented Apr 4, 2021

I think auto-wrapping any structs used as various kinds of buffers would be great.

Yeah, Vulkan shouldn't treat a 4 byte struct any different from a 128 byte one or whatever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: ark Issues that Ark depends on t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants