Stream: git-wasmtime

Topic: wasmtime / PR #7228 winch: Add known a subset of known li...

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

saulecabrera requested cfallin for a review on PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

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, 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

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

saulecabrera requested wasmtime-core-reviewers for a review on PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

saulecabrera requested fitzgen for a review on PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 17:08):

saulecabrera requested elliottt for a review on PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review comment:

I might call this index since it is the index within the BuiltinFunctionsArray right?

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review comment:

This is the offset within the vmctx of the whole BuiltinFunctionsArray?

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

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.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review comment:

Is there a reason not to use std::borrowed::Cow here?

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review comment:

Maybe name this unwrap_builtin?

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review 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.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:11):

fitzgen created PR review comment:

ditto

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:33):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2023 at 20:33):

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.

Wasmtime GitHub notifications bot (Oct 12 2023 at 21:03):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2023 at 21:03):

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)

Wasmtime GitHub notifications bot (Oct 12 2023 at 21:12):

saulecabrera updated PR #7228.

Wasmtime GitHub notifications bot (Oct 12 2023 at 21:13):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2023 at 21:13):

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 and get_info entirely.

Last updated: Nov 22 2024 at 16:03 UTC