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

[shaders] Expose a binding index set per target language #362

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

armansito
Copy link
Collaborator

vello_shaders exposes a list of resource binding types for each
compute stage. This list contains only the bindings that are
actually reachable (obeying WebGPU WGSL validation rules). The
list also contains only the types and no index information is
provided; instead the elements are provided in their order of
declaration in the shader source and the client is expected to
infer the index from the declaration order.

This leads to some issues that are unique to each target language:

  1. When targeting WGSL, this scheme is fragile if the shader source
    doesn't keep the index declarations contiguous. Any gaps in the
    indices will lead to incorrect results. This can happen due to a
    programmer error. This can also happen due to bindings that are
    unreachable by the entry point function, for example as a result
    of some refactoring.

  2. When targeting MSL, the client needs to know the vello_shader's
    crate's internal binding re-assignment scheme and implement the
    same logic on their end to compute the indices. This stems from
    the fact that WGSL bindings *where indices are scoped to a
    bind group) and MSL bindings (where indices are scoped separately
    by resource type) get assigned from different ranges and
    numerically not the same.

To resolve this, the crate now exposes a list of indices alongside each
backend source.

Additionally, as a cleanup, this PR removes all unused bindings and
rearranges the binding indices to leave no gaps.

Resolves #330

@armansito armansito requested review from raphlinus and dfrg September 14, 2023 02:47
@armansito armansito force-pushed the pr-issue-33 branch 2 times, most recently from b30617a to 9547988 Compare September 14, 2023 03:25
vello_shaders exposes a list of resource binding types for each
compute stage. This list contains only the bindings that are
actually reachable (obeying WebGPU WGSL validation rules). The
list also contains only the types and no index information is
provided; instead the elements are provided in their order of
declaration in the shader source and the client is expected to
infer the index from the declaration order.

This leads to some issues that are unique to each target language:

1. When targeting WGSL, this scheme is fragile if the shader source
   doesn't keep the index declarations contiguous. Any gaps in the
   indices will lead to incorrect results. This can happen due to a
   programmer error. This can also happen due to bindings that are
   unreachable by the entry point function, for example as a result
   of some refactoring.

2. When targeting MSL, the client needs to know the vello_shader's
   crate's internal binding re-assignment scheme and implement the
   same logic on their end to compute the indices. This stems from
   the fact that WGSL bindings *where indices are scoped to a
   bind group) and MSL bindings (where indices are scoped separately
   by resource type) get assigned from different ranges and
   numerically not the same.

To resolve this, the crate now exposes a list of indices alongside each
backend source.

Resolves linebender#330
clip_reduce, path_coarse_full, and fine stages declare some resource
bindings that are not used.

These handful of bindings have now been removed and the binding indices
were rearranged to be compact.

This partially helps with the bug reported in linebender#330. The actual fix for
that issue actually makes it so that the unused bindings are no longer
a problem with native integrations of the vello_shader crate, but this
is a reasonable clean up nontheless.
Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm curious to see how it will go for something with more complex bindings, like HLSL, but it seems reasonably straightforward so far.

@armansito
Copy link
Collaborator Author

This looks good to me. I'm curious to see how it will go for something with more complex bindings, like HLSL, but it seems reasonably straightforward so far.

HLSL is different but not drastically. Part of the idea with this change is that binding index representation is now separate for each backend language, so it's a (small) step towards supporting more of them. Not using multiple bind groups (thus descriptor sets/tables) definitely simplifies things for now.

@armansito armansito merged commit d4192ec into linebender:main Sep 15, 2023
4 checks passed
@armansito armansito deleted the pr-issue-33 branch September 15, 2023 20:32
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.

Potentially inaccurate clip_reduce bindings in shader build
2 participants