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

Helper macro for including precompiled SPIR-V #245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Oct 26, 2019

Should help suppress the temptation to court UB via naive include_bytes. Implementation placed in the root because macros are always imported via the root and it'd be weird for the returned type
to not be beside it.

@Ralith Ralith force-pushed the include-spv branch 5 times, most recently from 7458b50 to ae873e4 Compare October 26, 2019 02:42
Should help suppress the temptation to court UB via naive
include_bytes. Implementation placed in the root because macros are
always imported via the root and it'd be weird for the returned type
to not be beside it.
@MaikKlein
Copy link
Member

I don't fully understand the purpose of this. We can already use read_spirv with include_bytes to load shaders. This implementation copies to the stack which is not great.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 10, 2019

This can be used in a const context, which should not copy at all. In the cases where it does, LLVM will likely optimize it away. By contrast read_spirv always copies to the heap. The obscure nature of the Cursor API will further tend to encourage people without a firm grasp on UB to just transmute the slice when using include_bytes, so this provides a more convenient alternative.

@MaikKlein
Copy link
Member

This can be used in a const context, which should not copy at all.

Then it will do the stack allocation in a const context, which is probably also not what you want.

The obscure nature of the Cursor API will further tend to encourage people without a firm grasp on UB to just transmute the slice when using include_bytes, so this provides a more convenient alternative.

I think we should rather improve the API of read_spv then. I don't think many people will include their shaders in the binary, and if they do 1 allocation doesn't matter. And at one point read_spv might also become a const fn, although that probably takes a while.

rust also really should have a include_u32 or include_bytes_aligned, one can also implement this in a proc macro.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 10, 2019

Then it will do the stack allocation in a const context, which is probably also not what you want.

If you don't believe that will optimize it, you can use it to initialize a static and get a hard guarantee.

I think we should rather improve the API of read_spv then.

The API of that function is correct for what it's meant for, which is not fixing up include_bytes.

I don't think many people will include their shaders in the binary, and if they do 1 allocation doesn't matter.

People keep doing this and getting it wrong, and if the allocation doesn't matter then I'm not sure what the problem with the macro is. Some SPIR-V won't blow your stack, in the unlikely event that it ends up there to begin with.

rust also really should have a include_u32 or include_bytes_aligned, one can also implement this in a proc macro.

We've herein implemented this in a declarative macro, which has the substantial advantage of not requiring you to compile syn. Additionally, proc macros cannot yet provide equivalent path semantics as they don't know what file they've been invoked in.

@aloucks
Copy link
Contributor

aloucks commented Nov 11, 2019

I think this variant of the macro will work in a const context and should also guarantees against a copy on the stack:

#[macro_export]
macro_rules! include_spv {
    ($path:expr) => {{
        static ALIGNED: &'static $crate::util::Align4<[u8]> = &$crate::util::Align4(*include_bytes!($path));
        ALIGNED
    }};
}

And the example would be:

let vertex_code: &Spirv = include_spv!("../../shader/triangle/vert.spv");
let vertex_shader_info = vk::ShaderModuleCreateInfo::builder().code(vertex_code);

EDIT

I stand corrected. @Ralith is right, this won't work in a const context.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 11, 2019

I think this variant of the macro will work in a const context

It doesn't, which is exactly why you needed to replace const with let in the example. I'm not categorically opposed to this approach, but it seems like a reduction in generality for dubious benefit.

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.

None yet

4 participants