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

Replace spirv_std::storage_class::X<T> with T/&mut T and #[spirv(x)] &T/&mut T. #443

Merged
merged 3 commits into from
Mar 23, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Feb 22, 2021

This is my pre-#416 branch, I started this as part of #414, but only later realized I don't need it.

It's easier than I expected because, once again, I was able to mostly reuse existing logic (attribute parsing in this case).

As it's currently implemented, rules are:

  • only &T and &mut T are allowed as entry parameter types, with explicit storage class attributes
    • should probably be &mut MaybeUninitialized<T> instead of &mut T, but ideally we can use -> T instead for Output
    • it considers &T immutable but it should also check that T: Freeze
  • T defaults to Input, &mut T defaults to Output, and those storage classes cannot be explicitly specified
  • all other storage classes use a #[spirv(...)] attribute on the parameter, e.g. this line in sky-shader:
        #[spirv(push_constant)] constants: &ShaderConstants,
  • UniformConstant, and PushContant storage classes only allow &T (i.e. not &mut T) types
  • Private, Function and Generic storage classes cannot be specified at all
    • should probably restrict this further based on shader stage or kernel vs shader, but we can do that later

This works and passes tests, but I'm opening it as a draft because it should wait on a decision regarding #416, especially around the Output storage class and whether we should use a -> style, support bundling them in structs etc.

EDIT: updated to reflect PR changes, i.e. Input not taking references anymore.

@eddyb eddyb requested a review from khyperia February 22, 2021 15:41
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.

Looks hecking great! @hrydgard and I had a conversation in discord, path forward seems to be to make inputs be by-value, and then in a follow-up, do the annoyingly complex work of making outputs be returned via tuple or struct or whatever.

@eddyb

This comment has been minimized.

@XAMPPRocky XAMPPRocky mentioned this pull request Feb 25, 2021
@eddyb eddyb force-pushed the new-style-entry-interfac branch from 4c8c144 to ff92d4e Compare March 22, 2021 16:48
@eddyb eddyb marked this pull request as ready for review March 22, 2021 16:48
@eddyb eddyb changed the title Replace spirv_std::storage_class::X<T> with &T/&mut T and optionally #[spirv(x)]. Replace spirv_std::storage_class::X<T> with T/&mut T and #[spirv(x)] &T/&mut T. Mar 22, 2021
@VZout VZout removed their request for review March 22, 2021 17:05
@eddyb eddyb enabled auto-merge (rebase) March 22, 2021 20:30
@eddyb eddyb requested a review from khyperia March 22, 2021 20:30
@eddyb
Copy link
Contributor Author

eddyb commented Mar 22, 2021

@khyperia FWIW this is ready for a re-review, but I can't figure out what's wrong with the CI.
I don't want to tell it to retry because it might auto-merge because of the previous review.

@XAMPPRocky
Copy link
Member

I'm going to re-run and temporarily disable auto-merge

@XAMPPRocky
Copy link
Member

Yeah, it was an issue with GitHub Actions, the PR passes.

Comment on lines +301 to +302
// FIXME(eddyb) should we just remove all 5 of these storage class
// attributes, instead of disallowing them here?
Copy link
Member

Choose a reason for hiding this comment

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

Would there ever be a need to use these attributes in spirv-std? If not, I agree that we should remove them.

@XAMPPRocky XAMPPRocky merged commit 908487a into EmbarkStudios:main Mar 23, 2021
@eddyb eddyb deleted the new-style-entry-interfac branch March 23, 2021 11:07
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.

3 participants