Stream: git-wasmtime

Topic: wasmtime / PR #2541 x64 and aarch64: allow StructArgument...


view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30):

cfallin requested bnjbvr and julian-seward1 for a review on PR #2541.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 06:30):

cfallin requested bnjbvr and julian-seward1 for a review on PR #2541.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 04 2021 at 18:55):

bjorn3 submitted PR Review.

view this post on Zulip 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).

view this post on Zulip 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).

view this post on Zulip 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).

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr created PR Review Comment:

Could x64::lower::emit_vm_call be hoisted/split in a way that it could be reused, instead of having this specific helper?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr created PR Review Comment:

Here we should probably use the emit_vm_call.

view this post on Zulip 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")

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr created PR Review Comment:

Is the clone necessary here?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr created PR Review Comment:

Same question about alignment.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:40):

bnjbvr created PR Review Comment:

Can you add a comment referring to the legalize_signature function (or whatever is its new name), saying that it ensured its first retval register 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?

view this post on Zulip 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 index function we can use on iter() that would allow this to be a one-liner I think.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:47):

bjorn3 submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 12 2021 at 15:50):

bjorn3 submitted PR Review.

view this post on Zulip 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_call makes it much harder to prevent accidentally doing this I think.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip 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_regs is 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...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip 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!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin created PR Review Comment:

Fixed -- it should match any return value's purpose.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin created PR Review Comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin created PR Review Comment:

Unfortunately yes, otherwise this holds an immutable borrow on vcode for the duration of the loop while we also pass in &mut vcode to alloc_vregs() below. I considered turning some subset of the vcode inside-out somehow to work around this but it didn't seem worth it...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin created PR Review Comment:

Resolved as above :-)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:07):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 00:13):

cfallin submitted PR Review.

view this post on Zulip 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_call takes a LowerCtx and emits directly, which we need to do because the caller-side ABI object that it uses requires this; but gen_memcpy() returns a list of instructions because we cannot parameterize on the LowerCtx in 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 13:10):

bnjbvr submitted PR Review.

view this post on Zulip 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_conv hanging around, could we assert it's either equal to SystemV or Fastcall, just for future-proofing?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 13:12):

bnjbvr submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 13:13):

bnjbvr submitted PR Review.

view this post on Zulip 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?)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):

bnjbvr created PR Review Comment:

uber-nit: unimplemented! or todo! here?

view this post on Zulip 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?

view this post on Zulip 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?

view this post on Zulip 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 the gen_memcpy impl that the calling convention is one of a few known (e.g., not any of the Baldrdash ones).

view this post on Zulip 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.?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 15 2021 at 14:25):

bnjbvr created PR Review Comment:

(Looks like get_intreg_for_arg_system will check for the calling convention, so we should be fine!)

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):

cfallin created PR Review Comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:12):

cfallin submitted PR Review.

view this post on Zulip 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 i is in X-reg i), so no helper is defined, but I've created named let-bindings to make this a bit clearer.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):

cfallin created PR Review Comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:13):

cfallin created PR Review Comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 18 2021 at 07:50):

cfallin merged PR #2541.


Last updated: Dec 23 2024 at 13:07 UTC