JonasKruckenberg opened PR #8489 from JonasKruckenberg:no_once_lock
to bytecodealliance:main
:
This removes the dependency on
std::sync::SpinLock
by lifting the state out of a static and into theCallee
struct.This PR also removes the
call_conv
parameter since it was not used by any of the trait implementations.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
JonasKruckenberg requested elliottt for a review on PR #8489.
JonasKruckenberg requested wasmtime-compiler-reviewers for a review on PR #8489.
JonasKruckenberg edited PR #8489.
JonasKruckenberg edited PR #8489:
This removes the dependency on
std::sync::OnceLock
by lifting the state out of a static and into theCallee
struct.This PR also removes the
call_conv
parameter since it was not used by any of the trait implementations.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton commented on PR #8489:
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 commented on PR #8489:
This could possibly result in a performance degradation in the compiler: it removes the caching of the
MachineEnv
, which contains lists of registers (heap-allocatedVec
s), 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?
JonasKruckenberg commented on PR #8489:
Yeah thats a concern I had too.
The motivation is to support #8341 and sinceOnceLock
is the only thing in cranelift that cant be ported tono_std+alloc
or gated behind flags the idea was to replace it by something else.
Adding another dependency on something likespin
forno_std
targets doesn't seem appropriate either.I'll do some benchmarking next week and report back :+1:
cfallin commented on PR #8489:
Ah, one other option could be to put it in the
MachBackend
instead -- if it's the case that it no longer depends on thecall_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.
alexcrichton commented on PR #8489:
FWIW I ran this locally and on sightglass it reported a 1-3% slowdown in compiling spidermonkey.wasm, but no changes compiling other benchmarks. (I didn't re-run to see if that number was noise.
alexcrichton edited a comment on PR #8489:
FWIW I ran this locally and on sightglass it reported a 1-3% slowdown in compiling spidermonkey.wasm, but no changes compiling other benchmarks. (I didn't re-run to see if that number was noise)
jameysharp commented on PR #8489:
@uweigand introduced the
call_conv
parameter onget_machine_env
in #6957 with the intent to use it to have different sets of allocatable registers for the tail calling convention than for the system calling convention. We talked about that a little at the Cranelift meeting last week and it sounds to me like he still wants to use it that way, so I don't think we should remove it.Regarding the main point of this PR though: I believe we can get rid of the need for
OnceLock
here by changing regalloc2 soMachineEnv
holds a pair ofPRegSet
, which are small and const-evaluatable, instead of the sixVec
s. Then aMachineEnv
can be constructed in the initializer of astatic
item (note that Cranelift always setsfixed_stack_slots
to an emptyVec
, andVec::new
is a const fn). Then we can just return a borrow of a constant static in all of these cases, possibly choosing between several definitions based on the flags or calling convention.
uweigand commented on PR #8489:
@uweigand introduced the
call_conv
parameter onget_machine_env
in #6957 with the intent to use it to have different sets of allocatable registers for the tail calling convention than for the system calling convention. We talked about that a little at the Cranelift meeting last week and it sounds to me like he still wants to use it that way, so I don't think we should remove it.It's not completely certain, depending on choice of frame pointer register, but in general it would be good to have this option. Allocatable registers in general may depend on calling convention.
Last updated: Dec 23 2024 at 12:05 UTC