wasm-c-api icon indicating copy to clipboard operation
wasm-c-api copied to clipboard

Add support for `v128`

Open abrown opened this issue 6 years ago • 7 comments

As I was working on https://github.com/CraneStation/wasmtime/pull/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 avatar Sep 25 '19 19:09 abrown

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

abrown avatar Sep 25 '19 19:09 abrown

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.

jakobkummerow avatar Sep 25 '19 20:09 jakobkummerow

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?

rossberg avatar Sep 26 '19 13:09 rossberg

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?

abrown avatar Sep 26 '19 15:09 abrown

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

rossberg avatar Sep 26 '19 15:09 rossberg

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 https://github.com/WebAssembly/wasm-c-api/issues/68#issuecomment-535673429).
  • 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.

AndrewScheidecker avatar Sep 26 '19 20:09 AndrewScheidecker

@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.

rossberg avatar Oct 16 '19 07:10 rossberg