Stream: git-wasmtime

Topic: wasmtime / PR #7155 winch(x64): Add support for table ins...

Wasmtime GitHub notifications bot (Oct 05 2023 at 01:04):

saulecabrera opened PR #7155 from saulecabrera:winch-table-insts to bytecodealliance:main:

This change adds support for the following table insructions: elem.drop, table.copy, table.set, table.get, table.fill, table.grow, table.size, table.init.

This change also introduces partial support for the Ref WebAssembly type, more conretely the Func heap type, which means that all the table instructions above, only work this WebAssembly type as of this change.

Finally, this change is also a small follow up to the primitives introduced in https://github.com/bytecodealliance/wasmtime/pull/7100, more concretely:

• FnCall::with_lib: tracks the presence of a libcall and ensures that any result registers are freed right when the call is emitted.
• MacroAssembler::table_elem_addr returns an address rather than the value of the address, making it convenient for other use cases like table.set.

--

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 05 2023 at 10:59):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 11:37):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 12:55):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 13:57):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 05 2023 at 13:57):

saulecabrera created PR review comment:

I'm wondering if there's a chance we could make the signatures of the libcalls compatible with the Wasm spec, that way we would avoid having to pre-process the stack before emitting calls.

Wasmtime GitHub notifications bot (Oct 05 2023 at 13:58):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

saulecabrera requested cfallin for a review on PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

saulecabrera has marked PR #7155 as ready for review.

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

saulecabrera requested alexcrichton for a review on PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

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

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

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

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

saulecabrera requested wasmtime-default-reviewers for a review on PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 14:04):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7155.

Wasmtime GitHub notifications bot (Oct 05 2023 at 22:05):

saulecabrera requested elliottt for a review on PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt submitted PR review:

This looks great to me, I just had a few questions before approving.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt submitted PR review:

This looks great to me, I just had a few questions before approving.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

        // registers, since depending on the signature, the caller

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

Maybe this should be impl IntoIterator<Item = Val>? All the calls to it use .into_iter() on that argument.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

I think I'm missing something about this branch, won't it never be taken as elem_value != elem_value will be false?

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

Is the function guaranteed to never have a void return?

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

That seems like a reasonable change to me, @fitzgen is there any reason not to match the spec signature?

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:33):

elliottt created PR review comment:

This is a little surprising, shouldn't we always grow the stack by multiples of 16? (I'm assuming we call some sysv64 functions at some point, which requires 16-byte alignment for the stack.)

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:36):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:41):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:41):

saulecabrera created PR review comment:

Currently calls follow the system ABI requirements when it comes to alignment, but the alignment is done at the callsite, since we don't try to maintain alignment at all times (your stack could go unaligned if/when you perform any spills). Also this alignment will most certainly change to 16 once we support SIMD, but nevertheless we might still end up with an unaligned stack and so we would still need to perform at-callsite alignment. Hope this clarifies things.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:42):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:42):

saulecabrera created PR review comment:

Yeah, because this function should only be used to lazily initialize a funcref, we know that the builtin associated to it should always have 1 return value.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:48):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:48):

saulecabrera created PR review comment:

Yeah I agree this is confusing, and perhaps something that can be changed/improved because thinking a bit more about it, this is probably leaking some target-specific details. Internally masm.branch is going to emit a test if both registers are the same and if the comparison is zero or not zero (Eq, Ne). I'll make a note out of this, perhaps we can introduce either a new instruction at the MacroAssembler layer.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:58):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:59):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 18:59):

saulecabrera created PR review comment:

Thanks for the suggestion, fixed in https://github.com/bytecodealliance/wasmtime/pull/7155/commits/f4d975e4935ef46c48658bd6386707cdc8e6186d

Wasmtime GitHub notifications bot (Oct 06 2023 at 19:00):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 19:04):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 19:04):

saulecabrera created PR review comment:

Oops, I also left this here -- I'll remove it.

Wasmtime GitHub notifications bot (Oct 06 2023 at 19:05):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:20):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:20):

elliottt created PR review comment:

Would you mind adding a comment explaining the effect of the branch in the meantime? I think adding a test function to the macro assembler makes sense, but that doesn't need to block this PR.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:22):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:22):

elliottt created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/7179

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:23):

elliottt submitted PR review:

Looks good to me!

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:28):

saulecabrera updated PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:28):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:28):

saulecabrera created PR review comment:

Done, thanks!

Wasmtime GitHub notifications bot (Oct 06 2023 at 21:29):

saulecabrera has enabled auto merge for PR #7155.

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:58):

saulecabrera merged PR #7155.

Last updated: Dec 23 2024 at 12:05 UTC