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

Fix vqdmulhs_s32 native alias. #1257

Closed
wants to merge 3 commits into from

Conversation

Syonyk
Copy link
Contributor

@Syonyk Syonyk commented Jan 3, 2025

Function prototype has two parameters:
simde_vqdmulhs_s32(int32_t a, int32_t b)

Native define only had one. This does not build.
Tests only ever called with the simde_ prefix.

Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thank you for this, @Syonyk . I'm going to add more native alias testing to this PR before I merge it, so please don't push anymore to this branch.

@Syonyk
Copy link
Contributor Author

Syonyk commented Jan 3, 2025

Sounds good, I'm trying to keep my pull requests as simple and "one item" as possible, so you're free to do what you need with this branch.

@mr-c mr-c force-pushed the qdmulh_fix branch 7 times, most recently from 391b100 to ed9ea8d Compare January 4, 2025 02:29
Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

@Syonyk Once #1262 is merged I'll rebase this branch

The other reason this error wasn't found is that there are no tests for vqdmulhs_s32 ; can you add them?

@Syonyk
Copy link
Contributor Author

Syonyk commented Jan 13, 2025

Yeah, I'll sort something out for tests.

@Syonyk
Copy link
Contributor Author

Syonyk commented Jan 15, 2025

Test pushed, generated on the GCE ARM box. Though I admit I'm not sure how this tests the native alias, as the tests all use the simde_ prefix.

@mr-c
Copy link
Collaborator

mr-c commented Jan 16, 2025

Test pushed, generated on the GCE ARM box.

Thank you!

Though I admit I'm not sure how this tests the native alias, as the tests all use the simde_ prefix.

https://github.com/simd-everywhere/simde/blob/master/test/native-aliases.sh strips the simde_ prefix for the native test CI runs

Function prototype has two parameters:
simde_vqdmulhs_s32(int32_t a, int32_t b)

Native define only had one.  This does not build.
Tests only ever called with the simde_ prefix.
@mr-c
Copy link
Collaborator

mr-c commented Jan 16, 2025

With 59645b6 temporarily reverting your native alias fix, we should see confirmation that it worked at the native-aliases-arm64 CI check (that CI job should fail, since it lacks your fix)

and it indeed fails where it should in both the arm64 native aliases test and the amd64 native aliases test

@mr-c
Copy link
Collaborator

mr-c commented Jan 16, 2025

Fixed in f56ef45 ; thank you!

@mr-c mr-c closed this Jan 16, 2025
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.

2 participants