Skip to content

Add tests for every builtin #789

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

Merged
merged 1 commit into from
Nov 3, 2021
Merged

Add tests for every builtin #789

merged 1 commit into from
Nov 3, 2021

Conversation

khyperia
Copy link
Contributor

@khyperia khyperia commented Nov 3, 2021

In discord, it was pointed out that #[spirv(workgroup_size)] is broken - the decoration applies to a constant, not a variable, and it specifies the workgroup size, it's not provided with the workgroup size. (A subsequent discussion also showed that WorkgroupSize in SPIR-V is just plain broken with multi-entry-point shaders - but the LocalSize(Id) execution mode is a replacement for its behavior)

So, I decided to add a test for every builtin, to make sure nothing else is broken! turns out bool builtins are broken, I'll file a follow-up issue, and we had a fair amount of Kernel-only builtins which shouldn't be included.

Also, fixed a typo with layer_per_view_nv, and fixed opengl4.5 targeting SPIR-V 1.3 - spirv-val complains when trying to build with env opengl4.5, it's required to be 1.0.

Unfortunately, the test can't actually be added to CI, because compiletests is broken. Either @eddyb or I will hopefully be filing a fix soon. (both // ignore-vulkan1.1 and // only-vulkan1.1 are broken)

@khyperia khyperia requested a review from eddyb November 3, 2021 08:45
@khyperia khyperia mentioned this pull request Nov 3, 2021
@khyperia khyperia merged commit f7fc124 into main Nov 3, 2021
@khyperia khyperia deleted the all-builtins-test branch November 3, 2021 10:38
@expenses expenses mentioned this pull request Feb 7, 2022
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.

2 participants