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

Tracking issue for f32 and f64 methods in libcore #50145

Open
SimonSapin opened this issue Apr 21, 2018 · 29 comments
Open

Tracking issue for f32 and f64 methods in libcore #50145

SimonSapin opened this issue Apr 21, 2018 · 29 comments
Labels
A-floating-point Area: Floating point numbers and arithmetic A-intrinsics Area: Intrinsics C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

#49896 removes from libcore (and moves to libstd) three methods of f32 and f64 (that were only usable through the unstable trait core::num::Float) because they’re implemented by calling LLVM intrinsics, and it’s not clear whether those intrinsics are lowered on any platform to calls to C’s libm or something else that requires runtime support that we don’t want in libcore:

  • abs: calls llvm.fabs.f32 or llvm.fabs.f64
  • signum: calls llvm.copysign.f32 or llvm.copysign.f64
  • powi: calls llvm.powi.f32 or llvm.powi.f32

The first two seem like they’d be easy to implement in a small number of lower-level instructions (such as a couple lines with if, or even bit twiddling based on IEEE 754). abs in particular seems like a rather common operation, and it’s unfortunate not to have it in libcore.

The compiler-builtins crate has Rust implementations of __powisf2 and __powidf2, but in LLVM code those are only mentioned in lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp so I haven’t found evidence that llvm.powi.f32 and llvm.powi.f32 call those functions.

PR #27823 “Remove dependencies on libm functions from libcore” similarly moved a number of other f32 and f64 methods to libstd, but left these three behind specifically. (And unfortunately doesn’t discuss why.)

Maybe it’s fine to move them back in libcore? (As inherent methods, assuming #49896 lands.)

CC @alexcrichton

@SimonSapin SimonSapin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Apr 21, 2018
@alexcrichton
Copy link
Member

If we provide the fallback implementations in compiler-builtins I'd be fine moving these to libcore, but otherwise I'd personally prefer that they remain in std. I think the implementations in compiler-builtins may not be too bad though?

@Amanieu
Copy link
Member

Amanieu commented Apr 22, 2018

We could consider supporting all the math functions in compiler-builtins by essentially treating them the same way as memcpy: it's a required symbol, and we will provide a default implementation if you don't have one.

There no reason any of these functions (cos, tanh, etc) require anything outside of core, and it would eliminate the std/core split for floating-point types.

@SimonSapin
Copy link
Contributor Author

@Amanieu We certainly can consider that, but what I had in mind for this issue is starting for example with with powi where it looks like we already have an implementation in compiler-builtins. I just couldn’t figure out so far if/how we end up actually using it.

@Amanieu
Copy link
Member

Amanieu commented Apr 22, 2018

Regarding __powisf2, here is what I got from a grep of LLVM:

include/llvm/CodeGen/RuntimeLibcalls.def
118:HANDLE_LIBCALL(POWI_F32, "__powisf2")

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
4141:    Results.push_back(ExpandFPLibCall(Node, RTLIB::POWI_F32, RTLIB::POWI_F64,

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Apr 22, 2018

(Most) trigonometric and transcendental functions are far from trivial, especially if you want at all reasonable accuracy and performance (if not, people would -- rightly! -- avoid the libcore implementations or use them and find them unsatisfactory, defeating the purpose of providing them). They can be supported in a core-only environment in principle, but unless and until someone puts in the work to actually implement them, that's pie in the sky.

@vks
Copy link
Contributor

vks commented Apr 25, 2018

The three functions mentioned here (abs, signum, powi) are almost trivial to implement in pure Rust though (see num_traits).

@hellow554

This comment was marked as off-topic.

@SimonSapin
Copy link
Contributor Author

@vks Only having Rust implementations of those functions is certainly a possibility. But since LLVM intrinsics exist, should we try to use them? Maybe some architecture have dedicated instructions that can be faster?

On the other hand, can we rely on those intrinsics to be available for all targets, even when libm is not linked?

@hellow554 The reasons is that (as far as I know) we don’t yet have a clear answer to the questions above. Counting months is irrelevant, answers/solutions do now grow by themselves.

@hanna-kruppe
Copy link
Contributor

The LLVM intrinsics exist on every target, but many (all?) targets lower them to libm function calls. They are intrinsics mostly to facilitate optimizations, I believe.

Which leads to another potential problem: if we implement these in Rust, might LLVM optimizations canonicalize them to calls to the intrinsics? This is certainly a problem for other functions/intrinsics like memcpy, memset, etc. and although those problems can be solved with #![no_builtins], AFAIK that situation is subtly different than with the libm functions.

@varkor
Copy link
Member

varkor commented May 31, 2019

I believe this is also a problem for using the LLVM intrinsics for min and max on f32/f64 too (#18384), as they reside in libcore.

Edit: though am I understanding correctly that it's non-breaking to move a floating-point method from libcore to libstd?

bors added a commit that referenced this issue Jun 2, 2019
Update LLVM to include fmin/fmax optimisations

This will enable us to test if the optimisation issues mentioned in #18384 really are fixed. Unfortunately, using the intrinsics immediately is problematic due to the libcore/libstd split (see #50145).
bors added a commit that referenced this issue Jun 7, 2019
Use LLVM intrinsics for floating-point min/max

Resurrection of #46926, now that the optimisation issues are fixed. I've confirmed locally that #61384 solves the issues.

I'm not sure if we're allowed to move the `min`/`max` methods from libcore to libstd: I can't quite tell what the status is from #50145. However, this is necessary to use the intrinsics.

Fixes #18384.

r? @SimonSapin
cc @rkruppe @nikic
@varkor
Copy link
Member

varkor commented Jun 10, 2019

As per #61408, we should be able to use LLVM intrinsics provided we make sure they're properly supported.

  1. Ensure the LLVM intrinsic lowers to a suitable libm call.
  2. Make sure Rust libm supports the function (add it if not).
  3. Make sure compiler-builtins uses a version of libm that supports the function (update it if not).
  4. Make sure the version of compiler-builtins in rustc supports it (update it if not).
    compiler_builtins = { version = "0.1.16" }
  5. Use the intrinsic in libcore.

@Lokathor
Copy link
Contributor

Lokathor commented Aug 9, 2019

@varkor So I've been working on sqrt the past day or two.

  1. lowers to sqrt / sqrtf
  2. supported by libm
  3. not 100% exposed by compiler-builtins

But once the compiler-builtins issue gets resolved can we put sqrt in core?

@Lokathor
Copy link
Contributor

Update (see the above issue that references here): compiler-builtins needs no adjustment to support sqrt in core since LLVM will never itself emit a call to sqrt, so compiler-builtins and/or libm already has all bases covered.

How shall we proceed? Is this an RFC level change, or do we just PR to libcore and have T-libs approve it?

@varkor
Copy link
Member

varkor commented Aug 10, 2019

You should be able to simply open a PR, now that precedent has been set. I'd cc @alexcrichton in the PR.

@Lokathor
Copy link
Contributor

I suspect this will be much trickier than the small PRs I've done in the past, but I will attempt to get us started today or tomorrow.

@Urgau
Copy link
Member

Urgau commented Dec 18, 2021

To help this issue I've conduct a little experiment with compiler-explorer (godbolt) which consist of compiling the llvm intrinsics for every target that rust supports:

I think it's pretty safe to say that given the current situation fabsf64 and copysignf64 never call's libm.

Given this I would be willing to make a PR to move those functions back to core but I'm not sure this proves anything or that it will be usefull.

@Amanieu
Copy link
Member

Amanieu commented Dec 19, 2021

I don't think such a PR would be accepted. A better solution would be to properly support all math functions in core by providing them in compiler-builtins as weak functions.

cc @Lokathor who may have some thoughts on this.

@Lokathor
Copy link
Contributor

my understanding is that compiler-builtins is a deliberately minimal and "as needed" type of crate right now, rather than exhaustively providing all things.

Well it's trivial to copy the sign bit or clear the sign bit. LLVM can seemingly do that much on its own without ever asking for help. This assumes IEEE floats, but Rust already assumes that.

powf, not so much, I suspect that llvm will always need to call a libm here.

So I'm not against putting all functions into compiler-builtins all the time as float support in core is expanded, just so long as we're willing to recognize and document that llvm is exceedingly unlikely to ever call particular functions (and we provide them anyway just in case).

@Amanieu
Copy link
Member

Amanieu commented Dec 22, 2021

I just reviewed the past issues in compiler-builtins regarding support for weak functions, and here is what I propose we do:

  • Only provide libm functions and mem* functions on platforms that support weak symbols. Currently this is all ELF platforms. Non-ELF platforms must provide these functions from the OS libc. Currently non-ELF platforms are windows and darwin, so this shouldn't be a problem in practice.
  • Expose a libm Cargo feature (similar to the existing mem feature) to always export these functions as non-weak symbols, on all targets.

Once this is done, we should have the necessary symbols available on all targets which would allow moving the f32 and f64 methods into libcore.

@mati865
Copy link
Contributor

mati865 commented Dec 22, 2021

Technically LLVM supports using weak symbols on Windows, at least in mingw-w64 mode it works good. The problem is it requires whole comdat.

@lvella
Copy link

lvella commented Jul 6, 2023

Do we need an RFC for this or a PR is sufficient?

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2024

Cc @eduardosm

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2024

abs and copy_sign have pretty trivial implementations, so it seems odd that LLVM would even require a compiler-builtins implementation for them -- and from the experiments conducted above, it seems like indeed it does not require a compiler-builtins implementation. So what's preventing us from moving them to core?

I think they should be considered separately from other functions that actually require a non-trivial implementation, like pow or sqrt.

@tgross35
Copy link
Contributor

tgross35 commented Oct 5, 2024

I think it is also okay to move powi at this time since it is an intrinsic provided by libgcc or compiler-rt that we provide as a fallback via compiler-builtins - all of this is pretty well tested with no_std. The other math functions require some more work in our libm (something I am working on).

@scottmcm
Copy link
Member

scottmcm commented Oct 5, 2024

I think they should be considered separately from other functions that actually require a non-trivial implementation, like pow or sqrt.

IEEE 754 has a separate section "5.5.1 Sign bit operations" for copy/negate/abs/copySign, and they have extra freedom:

These operations may propagate non-canonical encodings.

Similarly, B.3 notes that negate, abs, and copySign are generally silent even on signalling NaNs.

So very much agreed for everything in that section, Ralf. It's like those ones are written intentionally to not need to think about most of the float complications, and to allow us to just do the integer implementation.


I think the "well we wrote it already" cases should be considered separately, Trevor. I think the sign-bit operations are fundamentally different from the others, in more than them already-working.

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2024

It's like those ones are written intentionally to not need to think about most of the float complications, and to allow us to just to the integer implementation.

Yes, exactly. They are also called out in the docs as "non-arithmetic operations", and they are the only non-arithmetic operations that are not in core.

@tgross35
Copy link
Contributor

tgross35 commented Oct 5, 2024

I think the "well we wrote it already" cases should be considered separately, Trevor. I think the sign-bit operations are fundamentally different from the others, in more than them already-working.

Well not literally the exact same PR :) just as far as how powi winds up being made available, it is more similar to softfloat mul/div than it is to most of the other math functions.

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2024

I made a PR to move abs, copysign, signum to libcore: #131304.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 14, 2024
float types: move copysign, abs, signum to libcore

These operations are explicitly specified to act "bitwise", i.e. they just act on the sign bit and do not even quiet signaling NaNs. We also list them as ["non-arithmetic operations"](https://doc.rust-lang.org/nightly/std/primitive.f32.html#nan-bit-patterns), and all the other non-arithmetic operations are in libcore. There's no reason to expect them to require any sort of runtime support, and from [these experiments](rust-lang#50145 (comment)) it seems like LLVM indeed compiles them in a way that does not require any sort of runtime support.

Nominating for `@rust-lang/libs-api` since this change takes immediate effect on stable.

Part of rust-lang#50145.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2024
Rollup merge of rust-lang#131304 - RalfJung:float-core, r=tgross35

float types: move copysign, abs, signum to libcore

These operations are explicitly specified to act "bitwise", i.e. they just act on the sign bit and do not even quiet signaling NaNs. We also list them as ["non-arithmetic operations"](https://doc.rust-lang.org/nightly/std/primitive.f32.html#nan-bit-patterns), and all the other non-arithmetic operations are in libcore. There's no reason to expect them to require any sort of runtime support, and from [these experiments](rust-lang#50145 (comment)) it seems like LLVM indeed compiles them in a way that does not require any sort of runtime support.

Nominating for `@rust-lang/libs-api` since this change takes immediate effect on stable.

Part of rust-lang#50145.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic A-intrinsics Area: Intrinsics C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests