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

PushConstant not being OpTypeStruct is invalid on Vulkan hosts #362

Closed
hrydgard opened this issue Jan 4, 2021 · 4 comments
Closed

PushConstant not being OpTypeStruct is invalid on Vulkan hosts #362

hrydgard opened this issue Jan 4, 2021 · 4 comments
Labels
a: push constant Issues related to push constants. a: vulkan Issues specifically about Vulkan. c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. t: bug Something isn't working

Comments

@hrydgard
Copy link
Contributor

hrydgard commented Jan 4, 2021

Quoth Baldur (hope he doesn't mind I post it here):

BTW I also noticed some invalid SPIR-V in your shader which may also affect this, you have a PushConstant storage class variable defined that is not an OpTypeStruct. All uniforms and push constant variables must be struct pointers with explicitly laid out structs.

@hrydgard hrydgard added the t: bug Something isn't working label Jan 4, 2021
@XAMPPRocky
Copy link
Member

XAMPPRocky commented Jan 5, 2021

Thank you for your issue! While I'm not doubting that is the case, I cannot find any reference in the SPIR-V spec that lays out that requirement. Perhaps they could expand further? @baldurk

The closest thing I could find is the following:

Composite objects in the UniformConstant, Uniform, and PushConstant Storage Classes must be explicitly laid out. The following apply to all the aggregate and matrix types describing such an object, recursively through their nested types

Where composite is defined as "An aggregate, a matrix, or a vector.", which would imply to me that this is not a requirement for those storage classes containing scalars.

@msiglreith
Copy link
Contributor

@XAMPPRocky That's probably referred to using Vulkan as host which comes with its own set of requirements and constraints:
https://vulkan.lunarg.com/doc/view/1.2.131.1/mac/chunked_spec/chap14.html#interfaces-resources-pushconst

@hrydgard
Copy link
Contributor Author

hrydgard commented Jan 5, 2021

That's a good catch @msiglreith !

Given how we want to use rust-gpu, having an option to, or at least initially, always following Vulkan's extra rules seems like the way to go.

@XAMPPRocky XAMPPRocky changed the title Slightly invalid SPIR-V (PushConstant not OpTypeStruct), according to BaldurK PushConstant not being OpTypeStruct is invalid on Vulkan hosts Jan 5, 2021
@XAMPPRocky XAMPPRocky added a: vulkan Issues specifically about Vulkan. c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. labels Jan 5, 2021
@XAMPPRocky XAMPPRocky added the a: push constant Issues related to push constants. label Mar 24, 2021
@khyperia
Copy link
Contributor

I believe this was fixed by two things:

  1. ABI rewriting shenanigans of rustc taking a push constant struct and replacing it with a scalar - fixed by abi: readjust FnAbis to remove unsupported PassModes, via query hooks. #766
  2. Push constants being used directly instead of structs (i.e. #[spirv(push_constant)] asdf: &f32) - fixed by wrapping everything in a struct Deprecate #[spirv(block)] and auto-wrap in "interface blocks" instead. #576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: push constant Issues related to push constants. a: vulkan Issues specifically about Vulkan. c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. t: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants