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 arrays of buffer descriptors #1148

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Firestar99
Copy link

@Firestar99 Firestar99 commented Apr 30, 2024

This PR adds support for arrays of buffer descriptors, and represents them like this in rust-gpu:

#[spirv(fragment)]
pub fn main(
    #[spirv(descriptor_set = 0, binding = 4, storage_buffer)] sized_buffer: &mut RuntimeArray<SizedBuffer>,
    #[spirv(descriptor_set = 0, binding = 5, storage_buffer)] unsized_buffer: &mut RuntimeArray<[u32]>,
) {}

This is a breaking change to how RuntimeArray operates. However, on all previous versions using it in such a way would result in a warning similarly to below. So I'm hoping this should have discouraged anyone from using it like that, and this change does not actually break anyone' code.

  warning: use &[T] instead of &RuntimeArray<T>
   --> example-shader/src/image_shader.rs:8:78
    |
  8 |     #[spirv(descriptor_set = 0, binding = 0, storage_buffer)] _sized_buffer: &mut RuntimeArray<SizedBuffer>,
    |                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  warning: `example-shader` (lib) generated 1 warning

But while trying to record that warning, I've noticed that even with a #![deny(warnings)] in the lib.rs of my shader crate, it would still only emit as a warning, thus when building via a build script not even show up to the end user.

Background

Currently we can represent the following glsl buffer declarations in rust-gpu:

layout(set = 0, binding = 0) buffer SizedBuffer {
    Mat3 model_matrix;
    uint id;
} sizedBuffer;

layout(set = 0, binding = 1) buffer UnsizedBuffer {
    uint data[];
} unsizedBuffer;

layout(set = 0, binding = 2) uniform image2D singleImage;
layout(set = 0, binding = 3) uniform image2D manyImages[];
pub struct SizedBuffer {
    pub model_matrix: glam::Mat3,
    pub id: u32,
}

#[spirv(fragment)]
pub fn main(
    #[spirv(descriptor_set = 0, binding = 0, storage_buffer)] sized_buffer: SizedBuffer,
    #[spirv(descriptor_set = 0, binding = 1, storage_buffer)] unsized_buffer: &mut [u32],
    #[spirv(descriptor_set = 0, binding = 2)] single_image: &Image!(2D, type=f32, sampled),
    #[spirv(descriptor_set = 0, binding = 3)] many_images: &RuntimeArray<Image!(2D, type=f32, sampled)>,
) {}

While we can have arrays of image or sampler descriptors, we cannot represent arrays of buffer descriptors, like we can in glsl. The proposed syntax for rust-gpu at the very top of this PR equals the following in glsl:

layout(set = 0, binding = 4) buffer SizedBuffer {
   Mat3 model_matrix;
   uint id;
} manySizedBuffers[];

layout(set = 0, binding = 5) buffer UnsizedBuffer {
   uint data[];
} manyUnsizedBuffers[];

@@ -2005,7 +2005,8 @@ impl<'a, S: Specialization> InferCx<'a, S> {
}

if let Err(e) = self.equate_match_findings(m) {
e.report(inst);
// temporarily disabled, arrays of buffer descriptors cause errors here
// e.report(inst);
Copy link
Author

@Firestar99 Firestar99 Apr 30, 2024

Choose a reason for hiding this comment

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

I cannot figure out why the specializer is upset about my changes (or what exactly it does in the first place). For now I've just disabled the error reporting, and the spirv emitted looks fine. So I assume this is a bug / missing feature in the specializer rather than my PR?
Any help would be appreciated :D

@tmvkrpxl0
Copy link

About concerns about breaking change, To me that warning is basically same as deprecated, use this instead. so I think it's fine to make breaking change.

@eddyb
Copy link
Contributor

eddyb commented Jul 11, 2024

Sorry if you didn't see this PR (which I should've landed a while back, oops!):

Crucially, it fully disambiguates the existence of a "buffer resource", and while it doesn't require it in the simple case, it is pretty important semantically.

The reason the SPIR-V types are weird and Block is needed, is because an OpTypeStruct with Block is closer to an OpTypeImage resource than to an actual data type. I really wish they had used pointer types instead, but it is what it is.

Using pointers (and ignoring the missing length for the outer one), the type of an array of buffer would be something like &[&[u32]], with the outer & being in the same address space as e.g. image types (UniformConstant in SPIR-V, WGSL does this better and calls it "handle" IIRC), and only the inner & being in the StorageBuffer address space (and pointing into the buffer).

Also, as another aside, for mutable buffers, the correct type would have to be something like &[&[AtomicU32]], because the outer & points into read-only memory (which just contains the buffer handles aka "descriptors", and if you could write to that memory, you could be corrupting the actual buffer handle representation etc.).

But for now, #1014 should work without breaking changes or ambiguous situations etc.

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