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

Adds encoding API and support for host functions with v128 values #562

Closed
wants to merge 7 commits into from

Conversation

mathetake
Copy link
Member

Signed-off-by: Takeshi Yoneda [email protected]

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
api/wasm.go Outdated Show resolved Hide resolved
api/wasm_test.go Outdated Show resolved Hide resolved
Comment on lines +517 to +518
// Host function takes two uint64 instead of a dedicated vector type in the signature.
mismatch = !expectedType.EqualsSignatureV128Flattened(actualType.Params, actualType.Results)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this seems weird special casing for host functions that want to take vector values as params or results ...

thoughts? @codefromthecrypt @anuraaga

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so basically with this we cannot judge, for example the host function func myfunc(uint64, uint64) should be imported for the signature either (v128) or (i64, i64) which we cannot disambiguate since we infer the host function's Wasm signature https://github.com/tetratelabs/wazero/blob/main/internal/wasm/gofunc.go#L240-L255

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though this is harmless :D

@mathetake mathetake changed the title Adds encoding API for vector values Adds encoding API for vector values / Support for host functions with vector types May 16, 2022
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake changed the title Adds encoding API for vector values / Support for host functions with vector types Adds encoding API and support for host functions with v128 values May 16, 2022
@codefromthecrypt
Copy link
Contributor

Vector types are prohibited in JavaScript, so there's no type binding there. Also V128 type != I128 (there's no i128 const). So, I want to check to see if there's any use case for vectorizing in host functions at all a sec, and that sounds suspicious as calling back to the host in a loop to use SIMD sounds backwards for perf. Anyway, I'll check and the result is that we may fail a signature rather than allow one.

@codefromthecrypt
Copy link
Contributor

@codefromthecrypt
Copy link
Contributor

Hi @mathetake I'm sorry that you did this work for possibly no reason. Can you pare this down so that the only thing we support publicly is global V128? probably high, low uint64 is the easiest way as unlike rust there's no u128 type in Go. Ex Get128(ctx) (high, low uint64)

Then inside host function documentation, mention that V128 is not a supported value type. In other words, we don't support binding, so we don't need to worry about marshaling of it until WebAssembly/spec#1474

@codefromthecrypt
Copy link
Contributor

ps if you want to keep the code for internal reasons, that's ok, just let's make it internal and impossible to use it in host functions with the api until the mentioned issue resolves.

@codefromthecrypt
Copy link
Contributor

sorry to reverse on this, but the spec lead clarified that there's no rule restricting v128 in general for both global and host function WebAssembly/spec#1474 (comment)

So, this leaves us with how to represent it. I think for starters in host functions use [16]byte for now, as unlikely anyone will use it anyway. Then, later someone could ask for a different representation. For globals, I think it is fine to use pair of uint64 as multi-return makes that fine.

My 2p

@codefromthecrypt
Copy link
Contributor

meanwhile the host-side vectorizing apis I really think we shouldn't add these as I can't imagine host side vectorization being anything except worse performance than without simd. I could be wrong and we could re-add it later if someone does that. Basically I think we can leave v128 opaque.

If folks feel strongly on keeping the host-side vectorization APIs, that's ok, just be careful to document this is unlikely to increase performance and could reverse it.

@codefromthecrypt
Copy link
Contributor

FWIW I searched as much as I could on github and could only find global v128 inside the wast itself. It makes me wonder really if it was the wast itself which was the use case leading to globals being supported. I may be jumping to conclusions also, just the lack of anything in GitHub is anchoring..

  (global $zero (mut v128) (v128.const i32x4 0 0 0 0))
  (func (export "v128.store8_lane_0")
    (param $address i32) (param $x v128) (result i64) (local $ret i64)
    (v128.store8_lane 0 (local.get $address) (local.get $x))
    (local.set $ret (i64.load (local.get $address)))
    (v128.store (local.get $address) (global.get $zero))    (local.get $ret))
;;snip

https://github.com/WebAssembly/spec/blob/main/test/core/simd/simd_store8_lane.wast#L6
ps why this is mutable no idea

Also, I found this source trying to figure out what the rules were:
https://github.com/WaterfoxCo/Waterfox/blob/current/js/src/jit-test/tests/wasm/simd/js-api.js

Here's the summary options we have (non exclusive)

  • add global it because the spec is clear that globals v128 should be supported
  • add vector intialization functions to the api (note: we can already pass a zero val!)
  • add host function support regardless of if other impls such as wasmtime do

my feeling is stick with basic globals and wait for someone to ask for the vectorization logic.

@codefromthecrypt
Copy link
Contributor

last thing.. all the questioning above are about host defined extern types. obviously functions need to support v128 for vectorization to work. so, the main q here is about adding to our public api functions like DecodeV128_I8x16 which agreed I asked about in the first place, just yeah the actual practical use is curious except matching parity with what you can do in wasm.

@mathetake
Copy link
Member Author

yeah agreed no usecase and wait unti someone asks for. Closing this and raise the Get128 API on the global in another PR!

@mathetake mathetake closed this May 16, 2022
@mathetake mathetake deleted the encodingsforvector branch May 16, 2022 23:41
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

Successfully merging this pull request may close these issues.

3 participants