wasmtime / PR #6067 winch(x64): Initial implementation fo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6067 winch(x64): Initial implementation fo...

Wasmtime GitHub notifications bot (Mar 20 2023 at 14:43):

saulecabrera opened PR #6067 from winch-x64-initial-calls to main:

This change adds the main building blocks for calling locally defined
functions. Support for function imports will be added iteratively after this
change lands and once trampolines are supported.

To support function calls, this change introduces the following functionality to
the MacroAssembler:

pop to pop the machine stack into a given register, which in the case of
this change, translates to the x64 pop instruction.

call to a emit a call to locally defined functions.

address_from_sp to construct memory addresses with the SP as a base.

free_stack to emit the necessary instrunctions to claim stack space.

The heavy lifting of setting up and emitting the function call is done through
the implementation of FnCall.

Wasmtime GitHub notifications bot (Mar 20 2023 at 14:43):

saulecabrera requested cfallin for a review on PR #6067.

Wasmtime GitHub notifications bot (Mar 20 2023 at 14:45):

saulecabrera edited PR #6067 from winch-x64-initial-calls to main:

This change adds the main building blocks for calling locally defined
functions. Support for function imports will be added iteratively after this
change lands and once trampolines are supported. Which is also when
we plan to introduce support for the Wasmtime* calling conventions.

To support function calls, this change introduces the following functionality to
the MacroAssembler:

pop to pop the machine stack into a given register, which in the case of
this change, translates to the x64 pop instruction.

call to a emit a call to locally defined functions.

address_from_sp to construct memory addresses with the SP as a base.

free_stack to emit the necessary instrunctions to claim stack space.

The heavy lifting of setting up and emitting the function call is done through
the implementation of FnCall.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:12):

alexcrichton updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:14):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:17):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:17):

saulecabrera created PR review comment:

The work by pop_to_reg and pop_to_named_reg was very similar, so I realized I could remove some repetition here through this refactor.

Wasmtime GitHub notifications bot (Mar 20 2023 at 15:17):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin created PR review comment:

Does this need to be a special case? It seems that if instead the spill_regs_in method accepted an empty range, this case would work with the code below...

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin created PR review comment:

This is a bit confusing, and I think a comment could help. First I had forgotten that "spill" in this context means reifying the state of the logical Wasm value stack onto the actual machine stack; then given that the top callee_params values are the args to the call, we want to put those last so they are actually in the right place. Perhaps something like
// "Spill" state to the machine stack. At this point, some of the logical Wasm stack may still be in
// registers. We need to save registers because they may be clobbered by the callee. At the same
// time, we also need to put parameters to the function on the stack. The logic below takes the
// current Wasm stack and partitions it into two parts: the values beyond the function args, which
// we just need to save; and the function args, which we save last so they are lowest on the stack
// (where our calling convention expects them).
or something like that?

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin created PR review comment:

It might be good to have a test initially that exercises the spilling behavior (here we can't because $add is called in tail position). Perhaps call it multiple times, with some values on the stack that live "under" the call?

Wasmtime GitHub notifications bot (Mar 21 2023 at 18:29):

cfallin created PR review comment:

A top-level module comment describing the calling convention would be really helpful here! In particular, which registers are clobbered or saved, and where arguments are passed.

Wasmtime GitHub notifications bot (Mar 27 2023 at 22:58):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:04):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:04):

saulecabrera created PR review comment:

I was not able to find a better way to do this. In this case we still want to spill everything, to save any live registers, but we are no interested in the count returned by spill_regs_and_count_memory_in, since they are not part of the function arguments so we can't claim that memory after the function call, so that's why I'm handling 0 in this way.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:04):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:04):

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:06):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:06):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:06):

saulecabrera created PR review comment:

Fixed! Thanks.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:06):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:06):

saulecabrera created PR review comment:

Agreed! Added the documentation in the abi module, and added a reference to it from here.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:07):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:24):

cfallin created PR review comment:

"Amount of registers" -> "number of registers"? (register is a count-noun so the "amount of" reads ambiguously to me: in bytes, or whole registers, or ...)

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:24):

cfallin created PR review comment:

I definitely appreciate the docs here, but a few things are slightly unclear to me at least:

The terminology of "memory entries" is confusing -- does this just mean "values on the stack"?

The "That is, any memory entries ... " sentence is really hard to parse. I think what this is saying is: values already on the stack that are args?

"Claim" means "reclaim" here?

Is it the case that memory_consumed + spill_stack_space == total_arg_stack_space? If so, could we use that alone to pop args off the stack? Fundamentally it seems like we shouldn't have to know where the args originally came from when we're cleaning up.

Wasmtime GitHub notifications bot (Mar 27 2023 at 23:24):

cfallin created PR review comment:

I'm a little confused here: shouldn't we be popping just total_arg_stack_space, without also popping memory_consumed and spill_stack_space? It seems like this is double-counting, unless I'm misunderstanding what the above amounts mean.

(Labeling portions of the stack in the figure might help clarify, too, or else noting the state of hte stack at points in the sequence above)

Wasmtime GitHub notifications bot (Mar 28 2023 at 00:15):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 28 2023 at 00:15):

saulecabrera created PR review comment:

Is it the case that memory_consumed + spill_stack_space == total_arg_stack_space?

I see where you're coming from and I agree that it's a bit confusing, I think I can make it so that it's more clear and so that we have to deal with less terms.

But to give a bit more context:

total_arg_stack_space is the total space that needs to be explicitly allocated at the callsite, and covers: the stack space needed for arguments plus any alignment needed for the caller's frame.

memory_consumed, represents the memory used by any Memory entries existing in the value stack at the moment of assigning those entries to the function arguments (in the case of a register argument, that memory will be loaded to the register used as argument, and in the case of a stack argument, that memory will be loaded to the scratch register and then stored to the particular argument slot). We could emit a pop instruction when doing this assignment and will potentially simplify things here in terms of what we are tracking, but I think it's more efficient to track the amount of memory used and reclaim it in the end.

spill_stack_space is the space created to save any live registers.

Does that clarify things to you?

As I write this, I believe we can simplify this by tracking all the memory used in a single field. Initially I thought that separating where things come from would make the code more understandable and easier to reason about, but maybe the opposite is better.

Wasmtime GitHub notifications bot (Mar 28 2023 at 00:51):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 00:52):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:15):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:16):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:17):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:17):

saulecabrera created PR review comment:

I've simplified how we track these values in https://github.com/bytecodealliance/wasmtime/pull/6067/commits/2e5372c8ffb430e75442ef33cc5cd7a3b408800e and also added more extensive documentation. Hopefully this clarifies the code.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:21):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:21):

saulecabrera created PR review comment:

Agreed, I've added a figure of how the stack looks like at each point in time in https://github.com/bytecodealliance/wasmtime/commit/2e5372c8ffb430e75442ef33cc5cd7a3b408800e, hopefully this helps clarify!

Wasmtime GitHub notifications bot (Mar 28 2023 at 01:21):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Mar 28 2023 at 10:25):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 13:03):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Mar 28 2023 at 13:22):

saulecabrera updated PR #6067 from winch-x64-initial-calls to main.

Wasmtime GitHub notifications bot (Mar 28 2023 at 18:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 28 2023 at 19:06):

cfallin merged PR #6067.

Last updated: Oct 23 2024 at 20:03 UTC