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

[spv-in] Support binding arrays #2199

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Conversation

Patryk27
Copy link
Contributor

@Patryk27 Patryk27 commented Jan 7, 2023

This pull request extends #1845 by implementing support for binding arrays for the SPIR-V frontend 🙂

In general, the changes here boil down to adjusting parse_type_array() so that it emits crate::TypeInner::BindingArray for texture-based arrays and sampler-based array -- as you'll see in the comments, the approach implemented here is kinda hacky, because - contrary to the wgls' frontend - when parsing OpTypeArray / OpTypeRuntimeArray we can't yet know whether it's going to be used through a binding or not.

I think that ideally instead of taking a guess in parse_type_array(), we'd adjust parse_global_variable() to do something like:

if let crate::TypeInner::Array { base, size, .. } = module.types[original_ty].inner {
    if dec.desc_set.is_none() || dec.desc_index.is_none() {
        let ty2 = crate::Type {
            name: None,
            inner: crate::TypeInner::BindingArray { base, size },
        };

        ty = module.types.insert(ty2, Default::default());
    }
}

... but that doesn't work, since it leaves the original type in module.types, causing validate_type() to fail later (because it always iterates through all of the types, even if they are not actually used anywhere, as would be the case here).

I'm currently writing a raytracer using rust-gpu and I've confirmed that a shader allowed by Naga with this patch actually works (on Linux + Nvidia, I haven't checked it on Windows or Mac).

Related to:
gfx-rs/wgpu#4313.

@Patryk27 Patryk27 force-pushed the spirv_binding_arrays branch from c2da6b8 to 881945e Compare January 7, 2023 18:21
Copy link
Member

@jimblandy jimblandy 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 great - thank you! Just some minor changes:

@Patryk27 Patryk27 force-pushed the spirv_binding_arrays branch from 881945e to 74d871f Compare January 10, 2023 09:08
@Patryk27
Copy link
Contributor Author

Patryk27 commented Jan 10, 2023

Okie, ready for round 2 🙂

@Patryk27 Patryk27 requested a review from jimblandy January 12, 2023 13:29
@Jerrody
Copy link

Jerrody commented Jan 24, 2023

@teoxoy @jimblandy
I think it can be merged!

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Lovely!

@teoxoy teoxoy merged commit 954cbaa into gfx-rs:master Jan 25, 2023
@Patryk27 Patryk27 deleted the spirv_binding_arrays branch January 25, 2023 18:11
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