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

specializer: allow unused fully-constrained interface variables. #729

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Aug 18, 2021

Fixes #723.

For a bit of background, this is what happens for e.g. an Input interface variable x: f32, during inference/specialization of OpTypePointer by storage class:

%f32_ptr<SC0: StorageClass> = OpTypePointer SC0 %f32
%x<SC0: StorageClass>: %f32_ptr<SC0> = OpVariable Input
    where SC0 = Input
OpEntryPoint ... %x

We're in a weird situation, where %x is fully determined to always be %x<Input>, because of the typing rule of OpVariable S: (OpTypePointer S _) (i.e. the redundancy of the storage class), but it still started out as "generic", and its type is an application of a "generic", so we have to treat it as such for consistency.

This isn't normally a problem, as any use of %x will be forced to instantiate %x<Input>, and so the OpEntryPoint will rely on that instance to replace its use of %x with %x<Input>.

However, if %x happens to be unused in the actual entry-point function, we would've panicked, before this PR, as the OpEntryPoint couldn't use any existing instances.

This PR works around that by "simply" instantiating any interface variables listed in OpEntryPoint, that have all of their "storage class parameters" fully constrained to concrete storage classes (and therefore require no inference/specialization).

@eddyb eddyb requested a review from khyperia August 18, 2021 16:48
use spirv_std as _;

#[spirv(closest_hit)]
pub fn test_closest_hit(#[spirv(object_to_world)] _object_to_world: [glam::Vec4; 3]) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to use a slightly modified test case, object_to_world requires an OpTypeMatrix on vulkan and so it's going to give an error when compiletest has target env of vulkan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah huh I haven't been running cargo compiletest with any flags lately because U thought Vulkan was the default.

@eddyb eddyb force-pushed the specializer-dead-vars branch from a14826d to 87cfce0 Compare August 19, 2021 10:15
use spirv_std as _;

#[spirv(closest_hit)]
pub fn test_closest_hit(#[spirv(object_to_world)] _object_to_world: [glam::Vec4; 3]) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I wrote the wrong code on #723.
I believe the type of _object_to_world should be [glam::Vec3; 4] (the same layout of Affine3A).
Since in glsl, gl_ObjectToWorldEXT[3][2] passes but emits out of range error when gl_ObjectToWorldEXT[2][3].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using that test anymore, because of #729 (comment), anyway.

@eddyb eddyb force-pushed the specializer-dead-vars branch from 87cfce0 to 1a8084e Compare August 19, 2021 14:29
@eddyb eddyb merged commit 9a69d6d into EmbarkStudios:main Aug 19, 2021
@eddyb eddyb deleted the specializer-dead-vars branch August 19, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler panics when using #[spirv(object_to_world)]
3 participants