-
Notifications
You must be signed in to change notification settings - Fork 356
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
miri: implement square root without relying on host floats #4026
Conversation
☔ The latest upstream changes (presumably #4029) made this pull request unmergeable. Please resolve the merge conflicts. |
bde6ebb
to
a8f30d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks! However, there are some parts I don't quite understand.
@rustbot author |
I added some comments, I hope it is more clear now. @rustbot ready |
16230fb
to
726efe0
Compare
Thanks a lot, great to finally have a proper implementation of the last missing standard IEEE float operation. :) |
(val * (F::from_u128(1).value + err).value).value | ||
} | ||
|
||
pub(crate) fn sqrt<S: rustc_apfloat::ieee::Semantics>(x: IeeeFloat<S>) -> IeeeFloat<S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is specified in 754 right? It should probably just go in https://github.com/rust-lang/rustc_apfloat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have an open issue for that: rust-lang/rustc_apfloat#14.
However, last time we wanted to change rustc_apfloat the answer was that things should land in LLVM first and then rustc_apfloat will pick them up from there. That's a quite tedious process. Not sure if that is still the current policy.
#3969 (comment)