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

Supporting OpRuntimeArray #503

Closed
XAMPPRocky opened this issue Mar 18, 2021 · 4 comments
Closed

Supporting OpRuntimeArray #503

XAMPPRocky opened this issue Mar 18, 2021 · 4 comments
Labels
a: compute Issues about compute shaders a: rust-lang Issues specific to rust-lang/rust. t: enhancement A new feature or improvement to an existing one.

Comments

@XAMPPRocky
Copy link
Member

At the last meeting it was brought up about what would be required to support OpTypeRuntimeArray. In that discussion we mostly the task into two separate parts supporting runtime data arrays, and supporting runtime descriptor arrays. The latter of which is more tricky and may require a new type, while the former might be better suited to being represented as a data slice [T].

@XAMPPRocky XAMPPRocky added t: enhancement A new feature or improvement to an existing one. a: rust-lang Issues specific to rust-lang/rust. a: compute Issues about compute shaders labels Mar 18, 2021
@Hentropy
Copy link
Contributor

User defined runtime data arrays are implemented in #504.

Adding support for slices is a matter of changing <PointeeTy as ConvSpirvType>::spirv_type to check if the type is a SpirvType::RuntimeArray and wrapping it in a struct if it is. However this causes two issues, both triggered by dependencies. The first is that the SpirvValues in the args argument to Builder::call are still just pointers directly to the RTA so assert_ty_eq fails. The second is that CodegenCx::create_const_alloc2 does not update the offset properly for ADTs with RTAs in them.

Runtime descriptor arrays, "static" descriptor arrays, and descriptors should all be handled together. There needs to be a special type for descriptor binding anyways to allow mutation of them since they are implicitly aliased.
Responsibilities of this type:

  • it needs to handle runtime descriptor arrays of runtime data arrays, ie. a DST of DSTs, which Rust does not directly allow
  • it must force the end user to index into the descriptor array and fail to type check if they do not
  • it must not allow calling .len() on the runtime descriptor array since the array size is not accessible to the shader
  • it must not allow direct safe mutable access to the data (there should be some special buffer types or something to help with this, but that is a different issue)
  • it should hold the set and binding numbers as const generics to prevent descriptor set number clashes in different libraries

I've been experimenting with syntax like this:

struct Slice<T>([T]);
fn my_shader(
    desc_rta_of_u8_rta: Bind<[Uniform<Slice<u8>>], 0, 0>,
)

Rust playground sketch, implemented in my fork see SpirvAttribute::Bind handling in entry.rs and abi.rs (note that straight slices still don't work, but user defined DSTs like in #504 do).

@khyperia
Copy link
Contributor

and wrapping it in a struct if it is

As of right now, I disagree that slices should be auto-wrapped in structs by the compiler. It causes a lot of issues (as you discovered), and I believe that if we do want to support straight-slice shader parameters (x: Uniform<&[f32]>, we don't want to right now), then we should synthesize a struct in the entry stub, then extract the field and pass in just the field into the user's main.

@expenses
Copy link
Contributor

expenses commented Apr 3, 2021

This is kinda tangential, but after #504, basic things like the following seem to work:

#[spirv(compute(threads(64)))]
pub fn main(
    #[spirv(global_invocation_id)] index: u32,
    #[spirv(uniform, descriptor_set = 0, binding = 0)] a: &RuntimeArray<f32>,
    #[spirv(uniform, descriptor_set = 0, binding = 1)] b: &RuntimeArray<f32>,
    #[spirv(uniform, descriptor_set = 0, binding = 2)] c: &mut RuntimeArray<f32>,

) {
    let index = index as usize;
    c.array[index] = a.array[index] * b.array[index];
}

pub struct RuntimeArray<T> {
    array: [T],
}

It does generate a lot of branching and OpUnreachables though due to the bounds checking. I couldn't get get_unchecked to work, and it required the Addresses capability anyway. Ideally something like this would be best:

unsafe fn get_runtime_array(array: &[f32], index: u32) -> f32 {
    let mut output = f32::default();

    asm! {
        "%f32 = OpTypeFloat 32",
        "%u32 = OpTypeInt 32 0",
        "%u32_0 = OpConstant %u32 0",
        "%runtime_array_f32 = OpTypeRuntimeArray %f32",
        "%runtime_array_f32_ptr = OpTypePointer Generic %runtime_array_f32",
        "%f32_ptr = OpAccessChain %runtime_array_f32_ptr {array_ptr} %u32_0 {index}",
        "%output = OpLoad %f32 %f32_ptr",
        "OpStore {output} %output",
        array_ptr = in(reg) array.as_ptr(),
        index = in(reg) index,
        output = in(reg) &mut output
    }

    output
}

However I got stuck with this error:

inference conflict: Concrete(IdRef(3)) vs Concrete(IdRef(1))
    in %64 = OpLoad 1 63

@msiglreith
Copy link
Contributor

Fixed by #596?

@khyperia khyperia closed this as completed Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: compute Issues about compute shaders a: rust-lang Issues specific to rust-lang/rust. t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants