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

push constants with a single type don't work #361

Closed
expenses opened this issue Dec 31, 2020 · 6 comments
Closed

push constants with a single type don't work #361

expenses opened this issue Dec 31, 2020 · 6 comments
Labels
a: push constant Issues related to push constants. t: bug Something isn't working

Comments

@expenses
Copy link
Contributor

The following 3 tests all fail:

#[test]
fn push_constant_single_vulkan() {
    val_vulkan(
        r#"
#[derive(Copy, Clone)]
pub struct ShaderConstants {
    pub time: f32,
}
#[allow(unused_attributes)]
#[spirv(fragment)]
pub fn main(constants: PushConstant<ShaderConstants>) {
    let _constants = constants.load();
}
"#,
    );
}
thread 'test::basic::push_constant_single_vulkan' panicked at 'Vulkan validation failed:
invalid id:0:0 - PushConstant OpVariable <id> '2[%constants]' has illegal type.
From Vulkan spec, section 14.5.1:
Such variables must be typed as OpTypeStruct, or an array of this type
  %constants = OpVariable %_ptr_PushConstant_float PushConstant
', crates/spirv-builder/src/test/mod.rs:109:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/std/src/panicking.rs:493:5
   1: std::panicking::begin_panic_fmt
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/std/src/panicking.rs:435:5
   2: spirv_builder::test::val_vulkan
             at ./src/test/mod.rs:109:9
   3: spirv_builder::test::basic::push_constant_single_vulkan
             at ./src/test/basic.rs:233:5
   4: spirv_builder::test::basic::push_constant_single_vulkan::{{closure}}
             at ./src/test/basic.rs:232:1
   5: core::ops::function::FnOnce::call_once
             at /home/ashley/.rustup/toolchains/nightly-2020-12-11-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
#[test]
fn push_constant_single_block_vulkan() {
    val_vulkan(
        r#"
#[derive(Copy, Clone)]
#[allow(unused_attributes)]
#[spirv(block)]
pub struct ShaderConstants {
    pub time: f32,
}
#[allow(unused_attributes)]
#[spirv(fragment)]
pub fn main(constants: PushConstant<ShaderConstants>) {
    let _constants = constants.load();
}
"#,
    );
}
error: `#[spirv(block)]` can only be used for `Sized` aggregates, but `ShaderConstants` has `Abi::Scalar(Scalar { value: F32, valid_range: 0..=4294967295 })`
   --> /home/ashley/projects/rust-gpu/crates/spirv-std/src/storage_class.rs:57:1
    |
57  | / storage_class! {
58  | |     /// Graphics uniform memory. OpenCL constant memory.
59  | |     ///
60  | |     /// Shared externally, visible across all functions in all invocations in
...   |
202 | |     #[spirv(physical_storage_buffer)] writeable storage_class PhysicalStorageBuffer;
203 | | }
    | |_^
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
#[test]
fn push_constant_value_vulkan() {
    val_vulkan(
        r#"
#[allow(unused_attributes)]
#[spirv(fragment)]
pub fn main(constants: PushConstant<f32>) {
    let _constants = constants.load();
}
"#,
    );
}
---- test::basic::push_constant_value_vulkan stdout ----
thread 'test::basic::push_constant_value_vulkan' panicked at 'Vulkan validation failed:
invalid id:0:0 - PushConstant OpVariable <id> '2[%constants]' has illegal type.
From Vulkan spec, section 14.5.1:
Such variables must be typed as OpTypeStruct, or an array of this type
  %constants = OpVariable %_ptr_PushConstant_float PushConstant
', crates/spirv-builder/src/test/mod.rs:109:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/std/src/panicking.rs:493:5
   1: std::panicking::begin_panic_fmt
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/std/src/panicking.rs:435:5
   2: spirv_builder::test::val_vulkan
             at ./src/test/mod.rs:109:9
   3: spirv_builder::test::basic::push_constant_value_vulkan
             at ./src/test/basic.rs:270:5
   4: spirv_builder::test::basic::push_constant_value_vulkan::{{closure}}
             at ./src/test/basic.rs:269:1
   5: core::ops::function::FnOnce::call_once
             at /home/ashley/.rustup/toolchains/nightly-2020-12-11-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/d32c320d7eee56706486fef6be778495303afe9e/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I think it's alright for the 3rd one to fail, using PushConstants with primitives seems a little dodgy to me, but it'd be nice to fix either test 1 or 2.

@expenses expenses added the t: bug Something isn't working label Dec 31, 2020
@charles-r-earp
Copy link
Contributor

Did you try #[repr(C)]? Yes, Rust will convert single and double field structs to scalar and scalar pairs.

@expenses
Copy link
Contributor Author

expenses commented Jan 1, 2021

If you add #[repr(C), you now get:

error: Cannot cast between pointer types
  --> src/lib.rs:17:22
   |
17 |     let _constants = constants.load();
   |                      ^^^^^^^^^^^^^^^^
   |
   = note: from: *{Function} struct ShaderConstants { time: f32 }
   = note: to: *{Function} u32

@XAMPPRocky
Copy link
Member

I think this is a duplicate of #362 since you mention using Vulkan, and the original error is the same as what's mentioned there.

@khyperia
Copy link
Contributor

Copying in some discussion from discord, the reason it goes from f32 to u32 in #[repr(C)] is that the call to load() has argument ABI PassMode::Cast. The fix is likely to create and hook a rustc query and revert the ABI back to PassMode::Direct or whatever.

@khyperia
Copy link
Contributor

Reported #373 for the failure about casting to u32, which is unrelated

@XAMPPRocky XAMPPRocky added the a: push constant Issues related to push constants. label Mar 24, 2021
@khyperia
Copy link
Contributor

I believe the ABI PassMode::Cast shenans part of this was fixed by #766. The PushConstant<f32> part (now written #[spirv(push_constant)] asdf: &f32) was I think fixed by #576, because it auto-wraps everything in a struct. Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: push constant Issues related to push constants. t: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants