saulecabrera requested cfallin for a review on PR #7228.
saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7228.
saulecabrera opened PR #7228 from saulecabrera:winch-final-builtins
to bytecodealliance:main
:
This change is a follow up to:
- https://github.com/bytecodealliance/wasmtime/pull/7155
- https://github.com/bytecodealliance/wasmtime/pull/7035
One of the objectives of this change is to make it easy to emit function calls at the MacroAssembler layer, for cases in which it's challenging to know ahead-of-time if a particular functionality can be achieved natively (e.g. rounding and SSE4.2). The original implementation of function call emission, made this objective difficult to achieve and it was also difficult to reason about.
I decided to simplify the overall approach to function calls as part of this PR; in essence, thecall
module now exposes a single functionFnCall::emit
which is reponsible of gathtering the dependencies and orchestrating the emission of the call. This new approach deliberately avoids holding any state regarding the function call for simplicity.This change also standardizes the usage of
Callee
as the main entrypoint for function call emission, as of this change 4Callee
types exist (Local
,Builtin
,Import
,FuncRef
), each callee kind is mappable to aCalleeKind
which is the materialized version of a callee which Cranelift understands.This change also moves the creation of the
BuiltinFunctions
to theISA
level given that they can be safely used accross multiple function compilations.Finally, this change also introduces support for some of the "well-known" libcalls and hooks those libcalls at the
MacroAssembler::float_round
callsite.--
prtest:full
<!--
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
-->
saulecabrera requested wasmtime-core-reviewers for a review on PR #7228.
saulecabrera requested fitzgen for a review on PR #7228.
saulecabrera requested elliottt for a review on PR #7228.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I might call this
index
since it is the index within theBuiltinFunctionsArray
right?
fitzgen created PR review comment:
This is the offset within the vmctx of the whole
BuiltinFunctionsArray
?
fitzgen created PR review comment:
In my experience, when we start to get many lifetime parameters like this, it helps to give them more descriptive names.
fitzgen created PR review comment:
Is there a reason not to use
std::borrowed::Cow
here?
fitzgen created PR review comment:
Maybe name this
unwrap_builtin
?
fitzgen created PR review comment:
I assume that the
base
andoffset
fields will be added together ultimately to doload(vmctx + base + offset)
and I wonder if it makes sense to just store thebase + offset
here, rather than the two parts separately.
fitzgen created PR review comment:
ditto
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Yeah, but note that this index is the built-in function's index multiplied by the pointer size, but index works for me.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
I don't think that this will work (unless I'm missing something). The reason why I'm storing both separately is that loading the builtin's address requires two loads:
- base = load(vmctx + base)
- builtin_addr = load(base + index)
saulecabrera updated PR #7228.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
As I was fixing this I realized that this was actually dead code, so I ended up removing
get_builtin
andget_info
entirely.
Last updated: Dec 23 2024 at 12:05 UTC