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 qdmlsl instructions #1195

Merged
merged 2 commits into from
Jan 4, 2025
Merged

Conversation

Ryo-not-rio
Copy link
Contributor

The qdmlsl instructions were implemented without any saturation. This has been fixed by utilising existing saturating instructions which are implemented correctly.

Unit tests have also been updated to test for saturation.

@Ryo-not-rio
Copy link
Contributor Author

Ryo-not-rio commented Jul 16, 2024

Looks like the high_n unit tests are incorrect, looking into it

@mr-c mr-c enabled auto-merge (squash) August 20, 2024 08:58
@mr-c
Copy link
Collaborator

mr-c commented Aug 20, 2024

Looks like the high_n unit tests are incorrect, looking into it

Thanks. I'm still seeing some test failures:

There is a compile failure with MSVC, maybe that will be helpful: https://ci.appveyor.com/project/nemequ/simde/builds/50436859/job/4lpievni4ibfsa7h#L2440 (our entire MSVC CI matrix failed, not just that version: https://ci.appveyor.com/project/nemequ/simde/builds/50436859)

@mr-c
Copy link
Collaborator

mr-c commented Sep 12, 2024

Ping @Ryo-not-rio ; any update on this or your other PR?

@mr-c
Copy link
Collaborator

mr-c commented Jan 3, 2025

@Syonyk If you get a chance, your feedback here would be appreciated

@Syonyk
Copy link
Contributor

Syonyk commented Jan 3, 2025

I believe these patches are correct, but were out of date with head. It looks like you've pulled them forward, so they now mostly have the changes required. I'm happy to take a closer look at this once I have my other fixes pushed, and I can test the updated files in my test suite as well.

If ryo-not-rio is not responding to requests to update tests and such, I can make a separate pull request with these changes, and add the proper test vectors. These functions were quite wrong before, though.

@mr-c mr-c disabled auto-merge January 3, 2025 22:52
@mr-c mr-c enabled auto-merge (squash) January 3, 2025 22:53
@mr-c
Copy link
Collaborator

mr-c commented Jan 3, 2025

Thank you @Syonyk ; looks like the previous compiler issues have been fixed so I've set this PR to automerge once the remaining CI checks are finished. Of course, if you still have feedback later that would be very welcome!

@mr-c
Copy link
Collaborator

mr-c commented Jan 3, 2025

Ha, I spoke too soon: MSVC is unhappy: https://ci.appveyor.com/project/nemequ/simde/builds/51262044/job/hojt0jq3999w07j7#L2451

../test/arm/neon/qdmlsl_high_lane.c(76): warning C4003: not enough arguments for function-like macro invocation 'simde_vqdmlsl_high_lane_s16'
../test/arm/neon/qdmlsl_high_lane.c(76): error C2440: 'function': cannot convert from 'simde_int16x4_t' to 'simde_int32x4_t'
../test/arm/neon/qdmlsl_high_lane.c(76): warning C4024: 'simde_vqdmlsl_s16': different types for formal and actual parameter 1
../test/arm/neon/qdmlsl_high_lane.c(76): error C2440: 'function': cannot convert from 'int' to 'simde_int16x8_t'
../test/arm/neon/qdmlsl_high_lane.c(76): warning C4024: 'simde_vget_high_s16': different types for formal and actual parameter 1
../test/arm/neon/qdmlsl_high_lane.c(76): error C2059: syntax error: ')'

@Syonyk
Copy link
Contributor

Syonyk commented Jan 3, 2025

Those look like valid errors... I can certainly take a look, but it won't be until next week. I'm nearing the end of my stare-at-screen time for the week.

Ryo-not-rio and others added 2 commits January 3, 2025 15:13
The qdmlsl instructions were implemented without any saturation.
This has been fixed by utilising existing saturating instructions which
are implemented correctly.

Unit tests have also been updated to test for saturation.

Change-Id: Ia9e5a7bd850bc178920c19e390c17db5a3bfbc4f
@mr-c
Copy link
Collaborator

mr-c commented Jan 4, 2025

Those look like valid errors... I can certainly take a look, but it won't be until next week. I'm nearing the end of my stare-at-screen time for the week.

No worries, I got a fix

@mr-c mr-c disabled auto-merge January 4, 2025 01:30
@mr-c mr-c merged commit 858b005 into simd-everywhere:master Jan 4, 2025
95 of 96 checks passed
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.

3 participants