Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Enable builtins.#848

Merged
eddyb merged 4 commits intoEmbarkStudios:mainfrom
dvc94ch:enable-builtins
Feb 9, 2022
Merged

Enable builtins.#848
eddyb merged 4 commits intoEmbarkStudios:mainfrom
dvc94ch:enable-builtins

Conversation

@dvc94ch
Copy link
Copy Markdown
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
Copy Markdown
Contributor

eddyb commented Feb 7, 2022

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

@dvc94ch
Copy link
Copy Markdown
Contributor Author

dvc94ch commented Feb 7, 2022

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

@dvc94ch
Copy link
Copy Markdown
Contributor Author

dvc94ch commented Feb 7, 2022

Is there an example of an existing builtin test somewhere?

@expenses
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

dvc94ch commented Feb 7, 2022

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

Copy link
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants