-
Notifications
You must be signed in to change notification settings - Fork 212
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
Faster float conversion operations #464
Conversation
Currently, compiler-builtins/src/float/conv.rs Lines 91 to 95 in 4117da3
I'm a bit confused by that. If The version in this PR does not have those special cases. I can put them back if someone thinks they are actually useful. |
I think it should be put back. |
Yes.. That's what I said. The special case is for a platform that does have a native version, which means this function wouldn't be called when someone does
Why? When would it be used on x86_64? |
Looks like it is also used in case of +soft-float. I guess that special case wouldn't be valid then though. |
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.
LGTM apart from a few style nits.
pub extern "C" fn __floatdidf(i: i64) -> f64 { | ||
// On x86_64 LLVM will use native instructions for this conversion, we | ||
// can just do it directly | ||
if cfg!(target_arch = "x86_64") { |
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.
Removing this is fine, I'm not even sure why this hack was here in the first place. If LLVM generates a native instruction then this function is never called anyways. Except in the case of soft-float, in which case you specifically don't want to use the built-in instruction.
Nice blog post; does it make sense to put any of these implementation in llvm's compiler-rt? |
@nickdesaulniers Sure! I don't have time for that, but if someone has time to port these implementations, that might benefit a lot of people :) |
This supersedes #370
This includes #463