Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30): Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30):cfallin opened PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30):cfallin requested bnjbvr and julian-seward1 for a review on PR #2541.
Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30):cfallin requested bnjbvr and julian-seward1 for a review on PR #2541.
Wasmtime GitHub notifications bot (Jan 04 2021 at 18:55):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 04 2021 at 20:54):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 04 2021 at 21:00):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 06 2021 at 02:10):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 06 2021 at 18:01):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Could
x64::lower::emit_vm_callbe hoisted/split in a way that it could be reused, instead of having this specific helper?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Is there an implicit invariant on the struct argument's size being a multiple of 8 or 16, so the callee can address it? If so, could we add an explicit assert here?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Here we should probably use the
emit_vm_call.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Is it wanted to a function returning multiple values, the second of which would be a struct return, would trigger the insertion below? (in other words: should this be "any return is the struct return")
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Is the clone necessary here?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Since this is a single-purpose legalization enforcing the presence of a StructReturn ret if there's an StructReturn input, how about naming it
ensure_struct_return_is_in_out, or something more targeted and precise?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Same question about alignment.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
light suggestion: could we use a newtype here (and below, where this comment is also repeated), explaining what the normalization is about?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
Can you add a comment referring to the
legalize_signaturefunction (or whatever is its new name), saying that it ensured its firstretvalregister is the struct return value?Also... would this unnecessarily add spill pressure for the entire function's lifetime to do it here (since one more register is blocked), instead of doing in the retval setup function?
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):bnjbvr created PR Review Comment:
There may be at most one struct arg, so we could break after this. There's even an
indexfunction we can use oniter()that would allow this to be a one-liner I think.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:47):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:47):bjorn3 created PR Review Comment:
Such an invariant shouldn't exist. Or at least cg_clif doesn't honour it. Maybe the abi requires it, in which case it would be nice to automatically add padding.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:50):bjorn3 submitted PR Review.
Wasmtime GitHub notifications bot (Jan 12 2021 at 15:50):bjorn3 created PR Review Comment:
It is important that the stack space for the outgoing arguments is not clobbered other than the stack space for the argument this should write. Using
emit_vm_callmakes it much harder to prevent accidentally doing this I think.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Good point; actually it could be not the first return value, so I've changed this to be more general and just find the StructReturn one (panic'ing if not present).
Re: register pressure, yes, possibly, though we're basically passing a value through from an arg to a return value so we either have to keep it in a vreg (note that
retval_regsis filled with vregs; the move to rax happens in the epilogue) or create a pseudo-spill-slot ourselves to save it; we can't recompute it later. So I don't think we can do better than this given the ABI, sadly...
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
I think there may be more than one struct arg? Struct return, yes, only one (unless we have multi-value with structs someday); but an arbitrary subset of the args may be struct types, I think.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
I considered this, but we mostly just hold onto it to return it back through the ABI trait, at which point we strip off the newtype wrapper anyway, so it didn't seem to add very much; happy to propagate it upward if you think that's best though!
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Fixed -- it should match any return value's purpose.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Done!
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Unfortunately yes, otherwise this holds an immutable borrow on
vcodefor the duration of the loop while we also pass in&mut vcodetoalloc_vregs()below. I considered turning some subset of the vcode inside-out somehow to work around this but it didn't seem worth it...
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Resolved as above :-)
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
My reading of the aarch64 spec is that on-stack args have 8-byte-aligned sizes; as far as I can tell the x64 SystemV spec also shows on-stack args broken down into "eightbytes". I've added an align-to-8 assert for both architectures.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):cfallin created PR Review Comment:
Yep, we need to be careful in exactly how we set up args and emit this call here, so I think it's best to open-code it.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:13):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 00:13):cfallin created PR Review Comment:
So I started down this path, but pulling on the string actually unravels a lot of things --
emit_vm_calltakes a LowerCtx and emits directly, which we need to do because the caller-side ABI object that it uses requires this; butgen_memcpy()returns a list of instructions because we cannot parameterize on theLowerCtxin the callee-side ABI object, because it is used as a dyn trait. This is slightly unfortunate, to say the least. If you think it will be a problem in practice then I can do this, but it requires altering the caller-side stuff to use more Vecs. In practice I think this will be OK because: struct args will only show up in SystemV and Fastcall cases; in both cases we can handle three register args; no stack adjustment or other ABI stuff should be necessary. Thoughts?
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:10):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:10):bnjbvr created PR Review Comment:
Ah, didn't see the LowerCtx trait coming! Okay, I'm convinced that this shouldn't be a problem for Fastcall too, so we can keep this code like this. If there's a
call_convhanging around, could we assert it's either equal toSystemVorFastcall, just for future-proofing?
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:12):bnjbvr created PR Review Comment:
I was looking for a way to document a bit better what this signature is about, and when it should be used in place of the Function's signature field. The newtype would make it semi-obvious, but it could be sufficient with a good comment here/on the getter too. Let's keep it as is.
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:13):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 13:13):bnjbvr created PR Review Comment:
Good point; actually it could be not the first return value, so I've changed this to be more general and just find the StructReturn one (panic'ing if not present).
Is there a test for this? (and if there's not, can you add one please?)
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
uber-nit:
unimplemented!ortodo!here?
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
nit: It really could be any pointer-typed temporary vreg here, right? This is only used to store the callee address, not an argument per se. It would require changing the signature of this function for a new temporary register. If your intent was to use one register, that happens to be fixed (because it's simpler than passing the temp) but while making sure it was unused by the other three, maybe rename the variable and add a comment here?
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
Can you add a test case with a call to a function that has two struct arguments, located at different offsets, please?
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
Just saw this TODO question: for the calling convention, there's actually
CallConv::for_libcall. For the rest of the signature, we should assert in thegen_memcpyimpl that the calling convention is one of a few known (e.g., not any of theBaldrdashones).
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
Is there, like on x64, some helper we could use to abstract "the first register argument", etc.?
Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):bnjbvr created PR Review Comment:
(Looks like
get_intreg_for_arg_systemwill check for the calling convention, so we should be fine!)
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin updated PR #2541 from struct-arg-ret to main:
This PR depends on #2538 and #2539 and includes those commits.
The StructReturn ABI is fairly simple at the codegen/isel level: we only
need to take care to return the sret pointer as one of the return values
if that wasn't specified in the initial function signature.Struct arguments are a little more complex. A struct argument is stored
as a chunk of memory in the stack-args space. However, the CLIF
semantics are slightly special: on the caller side, the parameter passed
in is a pointer to an arbitrary memory block, and we must memcpy this
data to the on-stack struct-argument; and on the callee side, we provide
a pointer to the passed-in struct-argument as the CLIF block param
value.This is necessary to support various ABIs other than Wasm, such as that
of Rust (with the cg_clif codegen backend).
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin created PR Review Comment:
Ah, that's more canonical, yes. Updated this and added asserts in x64/aarch64
gen_memcpy()that we aren't using a Baldrdash calling convention -- we should never be seeing struct args there anyway.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin created PR Review Comment:
Done!
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):cfallin created PR Review Comment:
The aarch64 ABI impl is a bit simpler in this regard (because arg
iis in X-regi), so no helper is defined, but I've created namedlet-bindings to make this a bit clearer.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):cfallin created PR Review Comment:
Done!
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):cfallin created PR Review Comment:
Done!
Wasmtime GitHub notifications bot (Jan 18 2021 at 07:50):cfallin merged PR #2541.
Last updated: Nov 22 2024 at 17:03 UTC