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

Add zero load opcodes #3943

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

Add zero load opcodes #3943

wants to merge 1 commit into from
+35 −9

Conversation

Zzzabiyaka
Copy link
Contributor

tested using a simple wat again

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch from 248d9fe to 1ea93cc Compare December 3, 2024 15:01
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch 2 times, most recently from b0202a7 to 072960f Compare December 23, 2024 06:44
@Zzzabiyaka
Copy link
Contributor Author

Hi @wenyongh @lum1n0us, could you take a look at this code please as @loganek should now be on Christmas holiday and the PR is quite simple in my opinion?

lum1n0us
lum1n0us reviewed Dec 23, 2024
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c Outdated
wasm_set_exception(module, "unsupported SIMD opcode");
break;
}
#define SIMD_LOAD_ZERO_OP(width) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

It appears there is some code shared with SIMD_LOAD_LANE_OP(like after int32 base...). Is there a way we can minimize code duplication?

Copy link
Contributor Author

@Zzzabiyaka Zzzabiyaka Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah that's a great idea actually

what we can do is call load_lane (0, width) to load the 0 lane and then nullify other lanes

needs some refactoring but doable

@Zzzabiyaka
Copy link
Contributor Author

@lum1n0us I refactored a bit, I think refactoring further will just bring a lot of complexity

@lum1n0us
Copy link
Collaborator

@lum1n0us I refactored a bit, I think refactoring further will just bring a lot of complexity

I'm quite fond of this version. It uncovers the logical connection between load_lane and load_zero.

lum1n0us
lum1n0us approved these changes Dec 23, 2024
View reviewed changes
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM

@Zzzabiyaka
Copy link
Contributor Author

looks like the CI failure is unrelated (it's also in classic-interp while the PR only touches the fast one) so I think it should be merge-ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lum1n0us lum1n0us lum1n0us approved these changes

@loganek loganek Awaiting requested review from loganek loganek 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.
3 participants
@Zzzabiyaka @lum1n0us @loganek