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

refactor(cranelift-codegen): remove OnceLock #8489

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JonasKruckenberg
Copy link
Contributor

@JonasKruckenberg JonasKruckenberg commented Apr 26, 2024

This removes the dependency on std::sync::OnceLock by lifting the state out of a static and into the Callee struct.

This PR also removes the call_conv parameter since it was not used by any of the trait implementations.

This removes the dependency on `std::sync::SpinLock` by lifting the state out of a static and into the `Callee` struct.
@JonasKruckenberg JonasKruckenberg requested a review from a team as a code owner April 26, 2024 10:02
@JonasKruckenberg JonasKruckenberg requested review from elliottt and removed request for a team April 26, 2024 10:02
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen labels Apr 26, 2024
@JonasKruckenberg JonasKruckenberg changed the title refactor(cranelift-codegen): remove SpinLock refactor(cranelift-codegen): remove OnceLock Apr 28, 2024
@alexcrichton
Copy link
Member

ping @elliottt (or other cranelift folks on this), I don't know enough about MachineEnv myself to know if the lift here is reasonable to apply

@cfallin
Copy link
Member

cfallin commented May 2, 2024

This could possibly result in a performance degradation in the compiler: it removes the caching of the MachineEnv, which contains lists of registers (heap-allocated Vecs), and recreates it for each function that is compiled. Especially for small functions this could be a measurable/non-negligible portion of the total work.

@JonasKruckenberg, could you say more about the motivation here? And have you measured the performance impact?

@elliottt elliottt removed their request for review May 2, 2024 20:43
@JonasKruckenberg
Copy link
Contributor Author

Yeah thats a concern I had too.
The motivation is to support #8341 and since OnceLock is the only thing in cranelift that cant be ported to no_std+alloc or gated behind flags the idea was to replace it by something else.
Adding another dependency on something like spin for no_std targets doesn't seem appropriate either.

I'll do some benchmarking next week and report back 👍

@cfallin
Copy link
Member

cfallin commented May 2, 2024

Ah, one other option could be to put it in the MachBackend instead -- if it's the case that it no longer depends on the call_conv. If it does on some backends (x64 might differentiate with fastcall?), we could statically construct the various options. That way, it's constructed once per compiler session, but without the lazy init.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants