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: readjust FnAbis to remove unsupported PassModes, via query hooks. #766

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Oct 12, 2021

Background

The rustc "call ABI" computation tries to find efficient ways to pass arguments (into functions) and return values (out of functions), and this can result in situations where e.g. PassMode::Cast is used to indicate that a [u8; 4] is actually being passed using a single 32-bit (CPU) register.

While useful at the machine ABI level, SPIR-V is too high-level and doesn't support the necessary raw manipulation to implement that kind of trick, so for a while now we've had to work around the situations in which rustc would decide for us on a PassMode that is incompatible with SPIR-V, with no room for the backend to override this behavior at all.

To a lesser extent, PassMode::Indirect is also not great for SPIR-V, as MIR allows more by-ref argument/return value passing than SPIR-V considers valid, and extra work was required to remove that after the fact (or bugs like #731 crept in because extra copies would've had to be used to remain correct).

Solution

We can now hook FnAbi computation, thanks to these upstream PRs:

The Layout/FnAbi interface refactors have already been accounted in rustup PRs (e.g. #759), so the main change in this PR is taking advantage of query hooking, to override fn_abi_of_* queries' behavior.

Both PassMode::Cast and (for good measure) PassMode::Ignore are now impossible, and if we wanted to we could also get rid of PassMode::Pair (like e.g. many C ABIs do), but that could introduce its own problems (so I would leave it for a different PR, if we even want it).


Fixes #373.

Fixes #731 (unrelated to PassMode::Cast, but because PassMode::Indirect is no longer used).

Note: opened as draft because I would like to add more tests before merging.

@eddyb eddyb requested a review from khyperia October 12, 2021 13:25
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.

aaah it's so beautiful, this is so good, I've been wanting this for soooo long ❤️

feel free to un-draft and merge whenever, and force-merge past the cargo-deny failure (it might be another day or two until that's fixed. hopefully.)

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs Outdated Show resolved Hide resolved
crates/rustc_codegen_spirv/src/codegen_cx/entry.rs Outdated Show resolved Hide resolved
@eddyb eddyb marked this pull request as ready for review October 20, 2021 18:31
@eddyb eddyb enabled auto-merge (rebase) October 20, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants