Skip to content

Fix Location calculation for arrays in entrypoint interface #513

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

Closed
wants to merge 4 commits into from

Conversation

Arc-blroth
Copy link

This PR fixes the calulation for Location decorations on array entrypoint parameters.

Both Vulkan (spec) and MLSL (page 44 of the spec) specify that array elements must take up consecutive locations, one for each element. Currently however, rust-gpu will always allocate 1 Location per entrypoint parameter, meaning that this code (playground link):

#[spirv(vertex)]
pub fn main_vs(
    data: Input<[f32; 4]>,
    more_data: Input<[f32; 4]>,
    mut out_data: Output<[f32; 4]>,
) {

will generate invalid SPIRV:

OpEntryPoint Vertex %1 "main_vs" %data %more_data %out_data
OpName %data "data"
OpName %more_data "more_data"
OpName %out_data "out_data"
OpDecorate %_arr_float_uint_4 ArrayStride 4
OpDecorate %data Location 0
OpDecorate %more_data Location 1
OpDecorate %out_data Location 0

since data and more_data have overlapping locations.

I'm not exactly that familiar with rustc and rustc_codegen_spirv, so there might be a better way to tell if an entrypoint parameter maps to a SPIRV array type than inspecting the generated type - please review :)

Copy link
Member

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! This looks good to me, can you also add two tests? One to ensure that the behaviour works, and one that generates the invalid entry param type error? You can see similar tests in tests/ui.

@Arc-blroth Arc-blroth force-pushed the fix/array_locations branch from c7ab407 to 386e7dd Compare March 23, 2021 02:13
@Arc-blroth
Copy link
Author

@eddyb pointed out that this PR is essentially blocked on #443 - I'll update this PR after that one is merged.

@khyperia khyperia added the s: waiting on author PRs that blocked on the author implementing feedback. label Apr 1, 2021
@Arc-blroth Arc-blroth force-pushed the fix/array_locations branch from 386e7dd to 2ea927e Compare April 5, 2021 03:10
Signed-off-by: Arc-blroth <[email protected]>
@Arc-blroth
Copy link
Author

Arc-blroth commented Apr 5, 2021

Sorry for taking so long, but I've updated to HEAD and added a test for fixing array locations. The CI seems to be failing because of commits that don't have any relationship to this PR, but I tested this PR and it works locally.

Copy link
Member

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

LGTM!

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, just adding a note to our future selves that this is incomplete, the vulkan spec says:

64-bit three- and four-component vectors consume two consecutive locations.

so we'll need to fix that too eventually (not needed in this PR).

@khyperia
Copy link
Contributor

Closing this for inactivity, if you'd like to revive this, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: waiting on author PRs that blocked on the author implementing feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants