Blocks (StorageBuffer and PushConstant)

[charles-r-earp]

Fixes #247. For compute shaders, more work like #282 is needed, but I believe that otherwise StorageBuffer as it is should be fine.

First, this PR adds an internal block decorated struct to StorageBuffer and PushConstant. This is necessary per spec, and certain platforms require it.

This creates a problem. When accessing the inner field, ie self.block.value, the resulting pointer has Function storage class. Idk if this is just a limitation or deficiency of the current method of applying storage classes (through the type system), but essentially the inner T type is typically user defined, and so can't be associated with a storage class (as it might not even be user defined but built in like an array, or reused in different places, say in glam). That means that it will have Function storage class. So despite the copy, you still end up with an access chain (ie in load / store) with mismatching storage classes, which may work on some systems but is incorrect and will fail validation. Essentially any reference that is obtained from a variable must have the same storage class, and any operations like access chain, load, store, etc, must inherit this as well.

It turns out that in this specific case, say PushConstant -> PushConstantBlock -> T (self.block.value), Rust emits a pointercast from *{PushConstant} PushConstantBlock to *{Function} T. I added logic to pointercast to apply the PushConstant storage to *T, but conservatively this only happens when the function is marked with the really_unsafe_ignore_bitcasts attribute. Note that this applies to all storage classes, including StorageBuffer.

Alternatively to the pointercast change, is to implement the access via asm! #254.

[charles-r-earp]

I was able to run this code, performing the operation y *= alpha. So with this PR, we'd be pretty close to being able to implement a lot of things. Note that we probably do want to be able to use runtime arrays instead of arrays, and need a way to access the array with a dynamic index. With a literal Rust invokes InBoundsAccessChain, but with a variable it creates a slice and then indexes that. Creating a slice requires Addresses since it uses intrinsics::offset. This will probably have to be fixed via asm!, to invoke access chain.

#![cfg_attr(target_arch = "spirv", no_std)]
#![feature(lang_items, repr_simd)]
#![feature(register_attr)]
#![register_attr(spirv)]

use spirv_std::storage_class::{Input, PushConstant, StorageBuffer};

// This could be imported from a shared module
#[derive(Clone, Copy)]
pub struct PushConsts {
    alpha: f32
}

#[allow(dead_code)]
#[allow(non_camel_case_types)]
#[derive(Clone, Copy)]
#[repr(simd)] // simd required to emit SPIR-V Vector
pub struct u32x3 {
    x: u32,
    y: u32, 
    z: u32
}

#[allow(unused_attributes)]
#[spirv(gl_compute(local_size_x=4))]
pub fn main(
    #[spirv(global_invocation_id)] global_id: Input<u32x3>,
    #[spirv(descriptor_set=1, binding=0)] mut y: StorageBuffer<[f32; 4]>,
    push_consts: PushConstant<PushConsts>
) {
    let gid = global_id.load().x;
    let alpha = push_consts.load().alpha;
    
    // match doesn't work yet
    // dynamic indexing doesn't work yet
    // Note how the loads and stores have to be ordered
    // because we are overwriting the entire array 
    // so this is entirely sequential
    // once you can modify just a single value or slice
    // this won't be necessary
    if gid == 0 {
        let mut x = y.load();
        x[0] *= alpha;
        y.store(x);
    } else if gid == 1 {
        let mut x = y.load();
        x[1] *= alpha;
        y.store(x);
    } else if gid == 2 {
       let mut x = y.load();
        x[2] *= alpha;
        y.store(x);
    } else if gid == 3 {
       let mut x = y.load();
        x[3] *= alpha;
        y.store(x);
    }
}

[khyperia]

This looks good, and is almost what our plans are for this area. In the future, though, could you collaborate with us to figure out what the best work is to do before submitting PRs? I hate to see any wasted work going down paths that we don't intend to go down.

[khyperia]

This comment isn't correct, right? It's the opposite of what's actually happening, it's only called in the ADT translation code. I believe this should be called at the entrypoint of trans_type_impl, in the same for loop that trans_image is called, and error when applied to a struct that results in a scalar or otherwise non-struct representation.

[khyperia]

While the #[spirv(block)] attribute matches our plans in this area for a hack quick fix, we were planning on having the user apply the attribute themselves to types used in block contexts. As you pointed out, there are many problems with this type-wrapping approach, surfacing most obviously with the storage class mismatch issues you're hitting.

(Do keep in mind that this is intended to be a quick hack fix before we're able to fully implement the bindings RFC, as that will replace this whole system with something much more elegant)

[khyperia]

Unfortunately this was a pretty high priority task (@eddyb started working on it yesterday, before you submitted this), and so was already merged (in #295). Really sorry about that :(

[charles-r-earp]

Unfortunately this was a pretty high priority task (@eddyb started working on it yesterday, before you submitted this), and so was already merged (in #295). Really sorry about that :(

No worries! I'm just happy this is a thing.