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

Add AccessQualifiers to Image, the Image! macro and codegen #1116

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Algorhythm-sxv
Copy link

Relates to #1097 .

This PR adds an extra generic parameter to the Image struct in spirv-std, extends the Image! macro to accept a new argument of the form access=[readonly|writeonly|readwrite] defaulting to readonly, and changes the codegen of SpirvType::Image to provide Some(access_qualifier) instead of None.

This change isn't quite ready to merge and requires some additional input to resolve remaining issues. Trying to compile a simple shader according to the rust-gpu dev guide immediately runs into issues:

Shader Code
#![no_std]

use spirv_std::glam::{uvec2, vec4, UVec3};
use spirv_std::{spirv, Image};

#[spirv(compute(threads(64)))]
pub fn main_cs(
    #[spirv(global_invocation_id)] id: UVec3,
    #[spirv(descriptor_set = 0, binding = 0)] image: &Image!(2D, format = rgba8, sampled = false, access = writeonly),
) {
    unsafe {
        image.write(
            uvec2(id.x, id.y),
            vec4(id.x as f32, id.y as f32, id.z as f32, 1.0),
        );
    }
}
build.rs
use spirv_builder::{MetadataPrintout, SpirvBuilder};

fn main() -> Result<(), Box<dyn std::error::Error>> {
    SpirvBuilder::new("shader_crate_name", "spirv-unknown-vulkan1.1")
        .print_metadata(MetadataPrintout::Full)
        .build()?;

    Ok(())
}

This setup fails to compile with the error:

 error: Operand 9 of TypeImage requires one of these capabilities: Kernel 
           %16 = OpTypeImage %float 2D 2 0 0 2 Rgba8 WriteOnly

However adding Capability::Kernel to the SpirvBuilder results in the following error, which also happens with spirv-unknown-vulkan1.2:

error: Capability Kernel is not allowed by Vulkan 1.1 specification (or requires extension)
           OpCapability Kernel

I am now out of my depth to be able to address this issue, but perhaps this is a hint as to why the access qualifier was not specified in the first place.

@Algorhythm-sxv Algorhythm-sxv requested a review from eddyb as a code owner January 13, 2024 19:36
@Patryk27
Copy link

Patryk27 commented Jan 16, 2024

Seems related: gfx-rs/naga#819 - it looks like instead of relying on that OpTypeImage's last argument, we should propagate this mutability information through elsewhere (no idea where, though).

@eddyb
Copy link
Contributor

eddyb commented Jan 31, 2024

@Patryk27 thanks, I completely miss this the first time! Found the Vulkan equivalents and left a comment on the original issue (#1097 (comment)), not seeing any obvious immediate action here (too many design choices).

@eddyb eddyb marked this pull request as draft February 27, 2024 07:51
@eddyb
Copy link
Contributor

eddyb commented Feb 27, 2024

Marked as draft because the current state is unlikely to happen (only relevant to OpenCL, see previous discussion in #1116 (comment) and in the issue at #1097 (comment)).

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.

3 participants