Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for v128 #106

Open
abrown opened this issue Sep 25, 2019 · 7 comments
Open

Add support for v128 #106

abrown opened this issue Sep 25, 2019 · 7 comments

Comments

@abrown
Copy link

abrown commented Sep 25, 2019

As I was working on bytecodealliance/wasmtime#330 I ran into some build issues due to the lack of v128 support in this library. I do have changes ready for supporting this new type but thought it best to discuss in an issue before opening a PR.

To add support for v128, my changes to wasm.h would consist of:

  • adding typedef __uint128_t v128_t; (it is debatable whether __uint128_t is the right type to use here)
  • adding a v128_t v128 field to the wasm_val_t union for storing the v128 bytes (any concerns with endianness?)
  • adding WASM_V128 to the wasm_valkind_enum for identifying the new type

Would this be sufficient to add the v128 type?

@abrown
Copy link
Author

abrown commented Sep 25, 2019

I should mention: @rossberg and @binji have discussed this indirectly in #66.

@jakobkummerow
Copy link
Collaborator

I don't think __uint128_t is a viable option: compilers for 32-bit platforms (x86 and arm) don't support it, and MSVC even for 64-bit doesn't support it either. In the interest of wide compatibility, I think using an array of smaller types (bytes, or maybe int32s) would be a better choice.

@rossberg
Copy link
Member

Adding a 128 bit field to wasm_val_t will double the size of all parameter vectors etc., so it's kind of undesirable -- and not a sustainable direction if the plan is to add even larger SIMD types later. As @binji suggested on #66, it might be best to represent such values via an indirection.

As an aside, SIMD is still a fair way from standardisation. I'm not sure we want to put it into the C API MVP. Create a branch for that perhaps?

@abrown
Copy link
Author

abrown commented Sep 26, 2019

SIMD is still a fair way from standardisation

Yeah, it does makes sense to wait until that happens. I guess we could leave this open to remember the discussion for when that happens. Combining both of your comments on the type itself, we are looking at something like typedef char[16] v128_t; and v128_t *v128 in the wasm_val_t enum, correct?

@rossberg
Copy link
Member

Something like that, plus suitable ownership management (which is where it gets nasty).

@AndrewScheidecker
Copy link
Contributor

If wasm_val_t contains an indirect pointer to wasm_v128_t, it would be essential that the caller is able to allocate the wasm_v128_t on the stack, and that ownership of that memory is not transferred to the callee. It would also be necessary to let the caller allocate stack memory for v128 results before making the call. It seems like it would make for a very error-prone API.

I think a better solution would be to get rid of wasm_val_t:

  • eliminate it from the ABI for calls into and out of WASM (see Exposing "regular" C/C++ functions? #68 (comment)).
  • change wasm_global_* to take a wasm_valkind_t and a void* instead of just a wasm_val_t*. Possibly add shortcuts like wasm_global_set_i32 etc.

@rossberg
Copy link
Member

@AndrewScheidecker, we don't know of any way to provide a generic and portably implementable call interface for functions in the absence of val_t. Also, automatic ownership management of reference values in the C++ API layer depends on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants