Skip to content

Add zero load opcodes#3943

Merged
loganek merged 1 commit intobytecodealliance:dev/simd_for_interpfrom
Zzzabiyaka:makslit/load_opcodes
Dec 23, 2024
Merged

Add zero load opcodes#3943
loganek merged 1 commit intobytecodealliance:dev/simd_for_interpfrom
Zzzabiyaka:makslit/load_opcodes

Conversation

@Zzzabiyaka
Copy link
Contributor

@Zzzabiyaka Zzzabiyaka commented Dec 3, 2024

tested using a simple wat again

Zzzabiyaka
Zzzabiyaka commented Dec 3, 2024
View reviewed changes
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/load_opcodes branch from 248d9fe to 1ea93cc Compare December 3, 2024 15:01
Zzzabiyaka
Zzzabiyaka commented Dec 4, 2024
View reviewed changes
@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

Zzzabiyaka commented Dec 23, 2024

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
Contributor

@lum1n0us lum1n0us Dec 23, 2024

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

Zzzabiyaka commented Dec 23, 2024

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

@lum1n0us
Copy link
Contributor

lum1n0us commented Dec 23, 2024

@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
Contributor

@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

Zzzabiyaka commented Dec 23, 2024

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

loganek
loganek approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@loganek loganek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@loganek
Copy link
Contributor

loganek commented Dec 23, 2024

The CI is green now after I re-ran it.

@loganek loganek merged commit dea195f into bytecodealliance:dev/simd_for_interp Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loganek loganek loganek approved these changes

@lum1n0us lum1n0us lum1n0us approved these changes

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

@wenyongh wenyongh Awaiting requested review from wenyongh

@xujuntwt95329 xujuntwt95329 Awaiting requested review from xujuntwt95329

@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