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

LibWasm: Fix sign issues in SIMD cmp ops #594

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

dzfrias
Copy link
Contributor

@dzfrias dzfrias commented Jul 12, 2024

All simd_*_cmp.wast now fully pass!

@dzfrias dzfrias requested a review from alimpfard as a code owner July 12, 2024 04:24
Comment on lines +293 to +294
SetSign<ElementType> lhs = result[i];
SetSign<ElementType> rhs = other[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 why would the signs be different? we already set them through the vector type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest with you... I have no idea why. I think it could be a number of things related to NativeVectorType and how the compiler does inference around it. Or we need to be explicit because the compiled native SIMD instructions aren't taking sign into account the way we wrote it before.

This does make around 100 tests pass, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm
I'll take a look later today, and merge if there's no cleaner solution.

@alimpfard alimpfard merged commit 4c7ef01 into LadybirdBrowser:master Jul 13, 2024
6 checks passed
@dzfrias dzfrias deleted the wasm-simd-cmp branch July 13, 2024 19:11
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.

None yet

2 participants