Skip to content

Conversation

@ospencer
Copy link
Member

No description provided.

@ospencer ospencer self-assigned this May 25, 2025
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

This looks awesome overall, I've just done an initial review so far, but want to go back over against the wasm api.

"build": [
"dune build @native --no-buffer"
],
"build": ["dune build @native --no-buffer"],
Copy link
Member

Choose a reason for hiding this comment

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

Why did the formatting change here?

from "runtime/unsafe/wasmv128" include WasmV128
use WasmV128.*

let (==) = (x, y) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why not provide this in the runtime module?

Copy link
Member

Choose a reason for hiding this comment

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

compiler/test/runtime might be a better place for this. I know the other unsafe tests exist here but we should honestly move those too. Especially as we would like to eventually split the runtime and stdlib.

| WasmSimdConstI8x16 =>
prim_type(
[
("lane0", Builtin_types.type_wasmi32),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would have been worth using a helper like List.init((index) => ("lane" ++ toString(index),Builtin_types.type_wasmi32), 15) for all of these.

};
let features =
if (Config.simd^) {
[Module.Feature.simd128, ...features];
Copy link
Member

Choose a reason for hiding this comment

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

Binaryen's default behaviour is to error if a feature isn't enabled but the code is used right?

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.

2 participants