Remove incorrect qrdmulh SSE code. #1258
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The only reason the SSE code for qrdmulh passed is because the edge cases were not included in the tests unless SSE was disabled. The INT16_MIN * INT16_MIN case ought result in INT16_MAX - and it does, in the fallback code. It does not, in the SSE code, which is what will typically be used on x86 hardware. Saturating code not handling edge cases is simply wrong.
|
Sounds good - if they're able to fix it properly, to cover all the test cases, I have no objections. Just so long as all cases are correct. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only reason the SSE code for qrdmulh passed is because the edge cases were not included in the tests unless SSE was disabled. The
INT16_MIN * INT16_MINcase ought result inINT16_MAX- and it does, in the fallback code. It does not, in the SSE code, which is what will typically be used on x86 hardware. Saturating code not handling edge cases is simply wrong.Unfortunately, my SSE skills are not sufficient to sort out how to fix this problem. I'd also be fine commenting out the code with a note about the problem, but I prefer correct results in all cases to "commonly correct results except in the case where the result matters."
Tests are modified to remove the conditional test. The final test cases passes cleanly on ARMv9 hardware, but not on x86 with SSE. It passes in the fallback code path.