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

Enable builtins. #848

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Enable builtins. #848

merged 4 commits into from
Feb 9, 2022

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Feb 7, 2022

No description provided.

@dvc94ch dvc94ch requested a review from eddyb as a code owner February 7, 2022 11:47
@eddyb
Copy link
Contributor

eddyb commented Feb 7, 2022

Can you try adding tests for these? I thought they couldn't work for now.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Feb 7, 2022

They seem to work fine. Any reason why you think they shouldn't?

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Feb 7, 2022

Is there an example of an existing builtin test somewhere?

@expenses
Copy link
Contributor

expenses commented Feb 7, 2022

See: #789 (I followed the git blame)

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)

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Feb 7, 2022

ok workgroup_size might be broken, realized I wasn't actually using it. but I think we went over this in #841, subgroup_local_invocation_id should be supported as it has the following enabling capabilities: Kernel, GroupNonUniform, SubgroupBallotKHR.

@hrydgard
Copy link
Contributor

hrydgard commented Feb 7, 2022

I think this should be fine to go in if you just back out the workgroup_size change, then.

@dvc94ch
Copy link
Contributor Author

dvc94ch commented Feb 7, 2022

Let me know if you want me to drop the example.

Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Looking at the Vulkan spec (since SPIR-V fails to document it), looks like SubgroupLocalInvocationId does indeed go on a variable.

(Whereas for WorkgroupSize it mentions "constants" - though technically ambiguous/a misuse of "variable")

@eddyb eddyb merged commit 4932a44 into EmbarkStudios:main Feb 9, 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.

4 participants