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.

Sign up for GitHub

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

Jump to bottom

Implement local and global variables handling for wasm simd #4056

Open
wants to merge 12 commits into
base: dev/simd_for_interp
Choose a base branch
from
Open

Implement local and global variables handling for wasm simd #4056

wants to merge 12 commits into from

Conversation

Zzzabiyaka
Copy link
Contributor

@Zzzabiyaka Zzzabiyaka commented Jan 27, 2025 •
edited
Loading

note:

I plan a bigger refactoring in a separate PR when we have all the spec tests passing so that I can easily check that there are no regressions from refactoring

@Zzzabiyaka Zzzabiyaka mentioned this pull request Jan 27, 2025
Closed
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 6 times, most recently from f9a278d to bf72fdb Compare January 27, 2025 14:50
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch from bf72fdb to 2bc58d6 Compare January 27, 2025 15:55
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch from d59ec3c to 8723135 Compare January 27, 2025 16:15
@Zzzabiyaka Zzzabiyaka mentioned this pull request Jan 27, 2025
Open
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 2 times, most recently from 952538c to c2909be Compare January 27, 2025 18:08
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch from 362946f to b8c9245 Compare January 27, 2025 18:26
loganek
loganek reviewed Jan 27, 2025
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c Outdated
@@ -3536,6 +3541,24 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
HANDLE_OP_END();
}

#if WASM_ENABLE_SIMDE != 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

That doesn't seem right - it makes the assumption about SIMD implementation; if for some platforms there will be another library used to implement SIMD instructions, you'd have to remember to update this place (and the condition will become more and more complicated). We should attempt to refactor the code so the simple and universal condition WASM_ENABLE SIMD != 0 should be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The same comment for all the other similar conditions in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's tackle this separately and out of this PR

loganek
loganek reviewed Jan 27, 2025
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c Outdated
addr = GET_OPERAND(uint32, I32, 0); \
frame_ip += 2; \
addr_ret = GET_OFFSET(); \
CHECK_MEMORY_OVERFLOW(4); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't it check the address of the v128 destination? if so, should that be 4 or rather 16?

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'll check this once more

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 3 times, most recently from 5450cae to 36833e4 Compare January 28, 2025 13:49
Zzzabiyaka
Zzzabiyaka commented Jan 28, 2025
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c
@@ -3536,6 +3541,24 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
HANDLE_OP_END();
}

#if WASM_ENABLE_SIMD != 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.

I know it's repetitive but let's do it in a separate PR (same for all the places where v128 repeats i64 a lot

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 10 times, most recently from cc9b7ae to dbbbd0a Compare January 28, 2025 21:34
Zzzabiyaka
Zzzabiyaka commented Jan 28, 2025
View reviewed changes
core/iwasm/libraries/simde/simde.cmake
if (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR "ARM.*")
add_definitions (-DWASM_ENABLE_SIMDE=1)
endif ()
add_definitions (-DWASM_ENABLE_SIMDE=1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should hopefully be gone after refactoring but keeping it here so far

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch from dbbbd0a to 8cae7cc Compare January 30, 2025 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loganek loganek loganek left review comments

@lum1n0us lum1n0us Awaiting requested review from lum1n0us lum1n0us is a code owner

@no1wudi no1wudi Awaiting requested review from no1wudi no1wudi is a code owner

@wenyongh wenyongh Awaiting requested review from wenyongh wenyongh is a code owner

@xujuntwt95329 xujuntwt95329 Awaiting requested review from xujuntwt95329 xujuntwt95329 is a code owner

@yamt yamt Awaiting requested review from yamt yamt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Zzzabiyaka @loganek