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

Remove shift of signed int in src_float_to_short_array #86

Merged
merged 2 commits into from
Aug 11, 2019

Conversation

Flamefire
Copy link
Contributor

  1. Scale by -SHORT_MIN
  2. Clip if float is outside range of short
  3. Otherwise round and truncate (guaranteed to fit to short)

Uses float instead of double as float can fit all 16 bit values.

Better alternative to approaches like #85, also adds test that show where e.g. janstary@ec07760 would fail.

Omits the clipping optimization which is normally disabled on x64 due to long being 64bits.

Scale by -SHORT_MIN
Clip if float is outside range of short
Otherwise round and truncate (guaranteed to fit to short)
Uses float instead of double as float can fit all 16 bit values
@erikd erikd merged commit 9225471 into libsndfile:master Aug 11, 2019
@Flamefire Flamefire deleted the shift_fix branch August 11, 2019 08:49
@Flamefire
Copy link
Contributor Author

After this was merged: Would it make sense to do the same for src_float_to_int_array? While it does not have the saved instruction when clipping optimization is disabled it would still look cleaner/more readable and avoid another configure check.

Sidenote: Unfortunately using lrint is a pessimization on remotely modern CPUs.:

  • lrint is required to handle overflow errors by setting ERRNO: http://www.cplusplus.com/reference/cmath/lrint/
  • without -fno-math-errno lrint won't get inlined but compiled to a library call due to the above making it slow
  • "modern" CPUs support the truncating rounding conversion of float/double->int in a single instruction

Docu: http://www.cplusplus.com/reference/cmath/lrint/
Experiment: https://godbolt.org/z/l77Mvt

@erikd
Copy link
Member

erikd commented Aug 11, 2019

How do we get the "truncating rounding conversion of float/double->int in a single instruction"?

@Flamefire
Copy link
Contributor Author

Simply use a plain cast. See the godbolt link for resulting assembly in comparison

@erikd
Copy link
Member

erikd commented Aug 11, 2019

Does a plain cast have the same rounding behavior as lrint? If it does not, its not a replacement.

@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 11, 2019

Yes and no. A cast is truncating so the same as "round to zero". So if the rounding mode is set to FE_TOWARDZERO then lrint does the same. Otherwise a "round to nearest" can be emulated by (x>0) ? x+.5f : x-.5f; which clang optimizes to a branch-less version on x86_64: https://godbolt.org/z/leAb8H

Note however that this "round to nearest" is not guaranteed when you use lrint.

A good read with comparisons on different archs: https://stackoverflow.com/a/37624488/1930508

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