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

implement local and function calls for v128 in the fast interpreter #4005

Open
wants to merge 1 commit into
base: dev/simd_for_interp
Choose a base branch
from

Conversation

Zzzabiyaka
Copy link
Contributor

add fast versions of local.set/get

add additional opcodes for these operations

add v128 values to handling of function params and return values

why?

These changes allowed us to run spec tests for simd as before that, functions with v128 params / return values could not be called

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch 7 times, most recently from 38895ee to 29181b5 Compare January 3, 2025 13:45
@@ -3536,6 +3541,24 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
HANDLE_OP_END();
}

#if WASM_ENABLE_SIMDE != 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#if WASM_ENABLE_SIMDE != 0 this should be #if WASM_ENABLE_SIMD != 0 but I'll fix it once we make the spec tests work and are ready to launch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think we might not need WASM_ENABLE_SIMDE flag in the first place, it probably should be set based on fast-interp + simd

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch 3 times, most recently from 84a5a0f to f72af42 Compare January 3, 2025 14:24
core/iwasm/interpreter/wasm_opcode.h Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_opcode.h Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_opcode.h Show resolved Hide resolved
core/iwasm/interpreter/wasm_loader.c Show resolved Hide resolved
core/iwasm/interpreter/wasm_interp_fast.c Show resolved Hide resolved
core/iwasm/interpreter/wasm_interp_fast.c Outdated Show resolved Hide resolved
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch 2 times, most recently from 667c8e1 to 0e0e2d0 Compare January 6, 2025 10:06
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch from 0e0e2d0 to 3562824 Compare January 6, 2025 10:08
@Zzzabiyaka Zzzabiyaka mentioned this pull request Jan 6, 2025
@Zzzabiyaka
Copy link
Contributor Author

looks like the CI failure is due to github infra and not my code, I think we're merge-ready

@@ -3536,6 +3543,24 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
HANDLE_OP_END();
}

#if WASM_ENABLE_SIMDE != 0
HANDLE_OP(EXT_OP_SET_LOCAL_FAST_V128)
HANDLE_OP(EXT_OP_TEE_LOCAL_FAST_V128)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file already quite bloated and difficult to maintain, I wonder if you considered refactoring it a bit? E.g. both EXT_OP_TEE_LOCAL_FAST_V128 and EXT_OP_COPY_STACK_TOP_V128 are very similar for the i64 opcode implementations.

#if WASM_ENABLE_FAST_INTERP != 0 && WASM_ENABLE_SIMDE != 0
EXT_OP_SET_LOCAL_FAST_V128 = 0xdd,
EXT_OP_TEE_LOCAL_FAST_V128 = 0xde,
EXT_OP_COPY_STACK_TOP_V128 = 0xdf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that EXT_OP_COPY_STACK_TOP_V128 is unneeded? It isn't emitted in wasm_loader.c.

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