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 ofFnCall
.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
saulecabrera requested cfallin for a review on PR #6067.
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 theWasmtime*
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 ofFnCall
.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
The work by
pop_to_reg
andpop_to_named_reg
was very similar, so I realized I could remove some repetition here through this refactor.
saulecabrera edited PR review comment.
cfallin submitted PR review.
cfallin submitted PR review.
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...
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?
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?
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.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera submitted PR review.
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 handling0
in this way.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Fixed!
saulecabrera submitted PR review.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Fixed! Thanks.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Agreed! Added the documentation in the abi module, and added a reference to it from here.
saulecabrera edited PR review comment.
cfallin submitted PR review.
cfallin submitted PR review.
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 ...)
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.
cfallin created PR review comment:
I'm a little confused here: shouldn't we be popping just
total_arg_stack_space
, without also poppingmemory_consumed
andspill_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)
saulecabrera submitted PR review.
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 apop
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.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera submitted PR review.
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.
saulecabrera submitted PR review.
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!
saulecabrera edited PR review comment.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
saulecabrera edited PR review comment.
saulecabrera updated PR #6067 from winch-x64-initial-calls
to main
.
cfallin submitted PR review.
cfallin merged PR #6067.
Last updated: Dec 23 2024 at 12:05 UTC