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

abi: explicitly unpack ScalarPair newtypes. #844

Merged
merged 1 commit into from
Jan 23, 2022

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Jan 23, 2022

I've left good description of the new codepath in a comment in the source, reproduced here:

Unlike Abi::Scalar's simpler newtype-unpacking behavior, Abi::ScalarPair can be composed in two ways:

  • two Abi::Scalar fields (and any number of ZST fields), gets handled the same as a struct { a, b }, further below
  • an Abi::ScalarPair field (and any number of ZST fields), which requires more work to allow taking a reference to that field, and there are two potential approaches:
    1. wrapping that field's SPIR-V type in a single-field OpTypeStruct - this has the disadvantage that GEPs would have to inject an extra 0 field index, and other field-related operations would also need additional work
    2. reusing that field's SPIR-V type, instead of defining a new one, offering the (a, b) shape rustc_codegen_ssa expects, while letting noop pointercasts access the sole Abi::ScalarPair field - this is the approach taken here

The user-facing summary is that MyNewtype<(u32, u32)> or MyNewtype<&[T]> (given struct MyNewtype<T>(T);) used to allow reading the single field MyNewtype by value (it would read the two scalars separately), but you couldn't take a reference to the field, as the types would be different OpTypeStructs (with the same fields).

On the implementation side, the choice was to keep the existing type identities for anything that itself has the two scalars as separate fields, and only reuse SPIR-V types in the "newtype of something that itself is ScalarPair" case, to avoid accidentally allowing more type conflation than necessary.

Fixes #836.

@eddyb eddyb enabled auto-merge (rebase) January 23, 2022 17:26
Copy link
Contributor

@hrydgard hrydgard left a comment

Choose a reason for hiding this comment

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

Looks good!

@eddyb eddyb merged commit 31e3fdb into EmbarkStudios:main Jan 23, 2022
@eddyb eddyb deleted the scalarpair-newtype branch January 23, 2022 19:47
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.

Cannot cast between pointer types
2 participants