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

Deprecate #[spirv(block)] and auto-wrap in "interface blocks" instead. #576

Merged
merged 8 commits into from
Apr 5, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Apr 5, 2021

As per Vulkan and OpenGL requirements:

  • PushConstant, Uniform and StorageBufer interface variables must have a Block-decorated OpTypeStruct type
    • this "struct" type is called an "interface block" and serves mostly as a grouping construct
    • as per Make #[spirv(block)] decoration force ABI type into Struct #391 (comment), having a struct (with some fields) inside an "interface block" shouldn't differ from having those same fields in the "interface block" itself
    • after this PR, we automatically generate a single-field "interface block" for these storage classes, instead of requiring the user to declare a struct and annotate it with #[spirv(block)]
  • Input/Output can also use "interface blocks", but it's not mandatory
    • we'd probably want to think a bit more about this once/if we have usecases in mind

Most of the commits in this PR are small refactors, leading up to the last commit, which is the actual change.

Because of #[spirv(block)] being deprecated instead of outright removed, this isn't a breaking change, except for slices, where you have to remove the struct wrapping the [T] that you previously had to add - it's now just:

#[spirv(storage_buffer, descriptor_set = 0, binding = 0)] slice: &mut [f32],

The auto-generated "interface block" also allows simpler uniforms like this to "just work":

#[spirv(uniform, descriptor_set = 1, binding = 0)] u_view_proj: &Mat4,

Closes #391.

EDIT: temporarily rebased on top of #577 to unbreak CI.

@fu5ha
Copy link
Member

fu5ha commented Apr 5, 2021

Very nice :D

@eddyb eddyb merged commit 4395b84 into EmbarkStudios:main Apr 5, 2021
@eddyb eddyb deleted the autoblock branch April 5, 2021 18:40
@eddyb
Copy link
Contributor Author

eddyb commented Apr 6, 2021

@khyperia @XAMPPRocky For the record I think this could be avoided in the future if GitHub has an option to require reviews from everyone who was added as a reviewer.

EDIT: can't find such an option in the branch protection settings of my own repos, welp.

Comment on lines +261 to +262
Decoration::Offset,
[Operand::LiteralInt32(0)].iter().cloned(),
Copy link
Member

Choose a reason for hiding this comment

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

What happens if there's multiple interface blocks in the function? Won't this point them to the same offset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just what you would get from this kind of struct:

#[spirv(block)]
struct InterfaceBlock<T> {
    inner: T,
}

So this isn't a regression, and I believe offsets are per interface variable.

There's an unrelated issue that we allow multiple push constants and multiple other bindings with the same (descriptor_set, binding), that's the only situations I can think of in which anything would overlap.

@@ -499,6 +516,12 @@ impl fmt::Debug for SpirvTypePrinter<'_, '_> {
.field("id", &self.id)
.field("image_type", &self.cx.debug_type(image_type))
.finish(),

SpirvType::InterfaceBlock { inner_type } => f
.debug_struct("SampledImage")
Copy link
Member

Choose a reason for hiding this comment

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

.debug_struct("InterfaceBlock")

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

LGTM

let len = bx
.emit()
.array_length(
len_spirv_type,
Copy link
Contributor

Choose a reason for hiding this comment

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

12 levels of indentation makes me sad :( would be nice to split this out

Copy link
Contributor Author

@eddyb eddyb Apr 6, 2021

Choose a reason for hiding this comment

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

I guess the newly introduced use of Builder means that we can interleave things a bit more and not have to separate them out so much - e.g. declare_interface_global_for_param could also return an OperandValue for the call args.

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.

Make #[spirv(block)] decoration force ABI type into Struct
4 participants