Skip to content

Conversation

@MaxGraey
Copy link
Member

@MaxGraey MaxGraey commented Jul 30, 2020

sync with musl's commit

The final rounding operation should be done with the correct sign
otherwise huge results may incorrectly get rounded to or away from
infinity in upward or downward rounding modes.

This affected sinh and sinhf which set the sign on the result after
a potentially overflowing mul. There may be other non-nearest rounding
issues, but this was a known long standing issue with large ulp error
(depending on how ulp is defined near infinity).

The fix should have no effect on sinh and sinhf performance but may
have a tiny effect on cosh and coshf.

In theory it shouldn't affect to us due to Wasm guarantee always have round-to-nearest mode. But It's cost nothing in term of performance and sync with musl's implementation at the same time so I guess it worth implement on our side as well.

  • I've read the contributing guidelines

@MaxGraey MaxGraey requested a review from dcodeIO July 30, 2020 14:23
@dcodeIO
Copy link
Member

dcodeIO commented Jul 30, 2020

In theory it shouldn't affect to us due to Wasm guarantee always have round-to-nearest mode.

So, is it safe to omit the fix: label here?

@MaxGraey
Copy link
Member Author

MaxGraey commented Jul 30, 2020

So, is it safe to omit the fix: label here?

yes. Removed it

@MaxGraey MaxGraey changed the title fix: sinh overflows in non-nearest rounding refactor: sinh overflows in non-nearest rounding Jul 30, 2020
@dcodeIO dcodeIO merged commit c286e06 into AssemblyScript:master Jul 30, 2020
@MaxGraey MaxGraey deleted the fix-math-sinh branch July 30, 2020 15:11
@dcodeIO
Copy link
Member

dcodeIO commented Jul 30, 2020

Thanks for looking into these upstream musl changes! :)

@github-actions
Copy link

🎉 This PR is included in version 0.14.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants