winch: Add a subset of known libcalls and improve call emission #7228
Conversation
This change is a follow up to: - bytecodealliance#7155 - bytecodealliance#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, the `call` module now exposes a single function `FnCall::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 4 `Callee` types exist (`Local`, `Builtin`, `Import`, `FuncRef`), each callee kind is mappable to a `CalleeKind` which is the materialized version of a callee which Cranelift understands. This change also moves the creation of the `BuiltinFunctions` to the `ISA` 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
saulecabrera
requested review from
cfallin and
fitzgen
and removed request for
a team
|
@elliottt this is the change that I mentioned earlier this week during the Cranelift weekly meeting. |
Subscribe to Label Action
This issue or pull request has been labeled: "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| /// The built-in function base, relative to the VMContext. | ||
| base: u32, |
There was a problem hiding this comment.
This is the offset within the vmctx of the whole BuiltinFunctionsArray?
There was a problem hiding this comment.
I assume that the base and offset fields will be added together ultimately to do load(vmctx + base + offset) and I wonder if it makes sense to just store the base + offset here, rather than the two parts separately.
There was a problem hiding this 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
changed the title
| } else { | ||
| todo!("libcall fallback for rounding is not implemented") | ||
| FnCall::emit::<Self, Self::Ptr, _>(self, context, |context| { |
winch/codegen/src/codegen/context.rs
Outdated
| @@ -27,25 +28,37 @@ use std::ops::RangeBounds; | |||
| /// generation process. The code generation context should | |||
| /// be generally used as the single entry point to access | |||
| /// the compound functionality provided by its elements. | |||
| pub(crate) struct CodeGenContext<'a> { | |||
| pub(crate) struct CodeGenContext<'a, 'b: 'a> { | |||
There was a problem hiding this comment.
Does 'b actually outlive 'a here, or would it be possible to collapse these down to just 'a?
There was a problem hiding this comment.
It actually outlives 'a; the changes in this PR make it so the compiled_function function at the ISA level gets a reference to BuiltinFunctions, which will outlive any of the other structs created in the body of the compile_function function (e.g. FuncEnv and VMOffsets) and on top of that, there's is a third lifetime ('data, previously 'c) which represents the lifetime of any references to the original WebAssembly binary through wasmtime_environ::ModuleTranslation.
| @@ -54,15 +55,13 @@ pub struct CalleeInfo { | |||
| /// | |||
| /// Contains all information about the module and runtime that is accessible to | |||
| /// to a particular function during code generation. | |||
| pub struct FuncEnv<'a, P: PtrSize> { | |||
| pub struct FuncEnv<'a, 'translation: 'a, 'data: 'translation, P: PtrSize> { | |||
There was a problem hiding this comment.
Similarly to the CodeGenContext parameters, can this be simplified down to using the 'a lifetime for everything?
There was a problem hiding this comment.
Yeah, the same comment applies here too.
saulecabrera
force-pushed
the
winch-final-builtins
branch
from
09c846f
to
216ffc3
Compare
This change is a follow up to:
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, the
callmodule now exposes a single functionFnCall::emitwhich 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
Calleeas the main entrypoint for function call emission, as of this change 4Calleetypes exist (Local,Builtin,Import,FuncRef), each callee kind is mappable to aCalleeKindwhich is the materialized version of a callee which Cranelift understands.This change also moves the creation of the
BuiltinFunctionsto theISAlevel 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_roundcallsite.--
prtest:full