Remove dependencies on libm functions from libcore. #27823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
I feel ok about this direction. |
|
Looks like some parts of coretest are failing (from travis): |
| #[cfg(target_env = "msvc")] | ||
| fn floorf(f: f32) -> f32 { (f as f64).floor() as f32 } | ||
| #[cfg(not(target_env = "msvc"))] | ||
| fn floorf(f: f32) -> f32 { unsafe { intrinsics::floorf32(f) } } |
There was a problem hiding this comment.
Was this msvc/not-msvc logic intentionally left out of the transition?
There was a problem hiding this comment.
Ah I see what happened, it looks like everything is relegated to cmath now instead of the intrinsics.
I have a feeling we probably want to continue to keep calling these intrinsics. Although they're lowered down to calls to actual library functions they probably exist to enable some form of optimizations in LLVM by having semantic information about what's going on.
There was a problem hiding this comment.
I replicated the logic; see https://github.com/rust-lang/rust/pull/27823/files#diff-5d0a51e064ef7bf53a7012c0b553d83aR116 .
There was a problem hiding this comment.
Right, but they're no longer calling the intrinsics (instead they're directly making the FFI calls)
There was a problem hiding this comment.
I doubt it makes much of a difference, but I can change the code to call the intrinsics.
eefriedman
force-pushed
the
float-dep-core
branch
from
4711341 to
3cfcf69
Compare
|
Updated to address review comments. |
|
@bors: r+ 3cfcf69883476e559455e02bc7635d174c627d11 Thanks! |
|
⌛ Testing commit 3cfcf69 with merge b6dc22d... |
|
💔 Test failed - auto-win-msvc-64-opt |
eefriedman
force-pushed
the
float-dep-core
branch
from
3cfcf69 to
ca848a5
Compare
|
Updated to fix compile failure on MSVC. |
|
@bors r=alexcrichton |
|
📌 Commit ca848a5 has been approved by |
|
⌛ Testing commit ca848a5 with merge ec9d362... |
|
💔 Test failed - auto-win-msvc-32-opt |
eefriedman
force-pushed
the
float-dep-core
branch
from
ca848a5 to
fef54bf
Compare
|
Attempted fix; apparently f32 Rem doesn't actually work with MSVC targets, and nobody noticed until now. |
| // The builtin f32 frem doesn't compile when targeting | ||
| // msvc; use f64 rem instead. | ||
| // FIXME: This is a bug; trans should be fixed instead | ||
| // of working around it here. |
There was a problem hiding this comment.
Perhaps this comment could stay referencing Float::floor? This is basically the same fix where the header files for MSVC has fmodf as just a wrapper around fmod, so the comment over there is the same for here. It's also not clear that trans should be fixed so much as LLVM itself.
There was a problem hiding this comment.
Given that the intrinsic isn't actually explicitly referenced here, pointing at floor() would be confusing at best. Also, it's impossible to fix #27859 here.
There was a problem hiding this comment.
It's not explicitly using an intrinsic but it's the same idea, LLVM is lowering this operation to a function call which isn't implemented on MSVC (explaining the #[cfg] block).
The current comment is somewhat odd with a reference to "frem" (requires knowledge of compiler internals) and has a FIXME which isn't actionable (needs to be altered in upstream LLVM perhaps).
The relevant comments (as well as explaining what LLVM does) are all available in std::f32::floor now
eefriedman
force-pushed
the
float-dep-core
branch
from
fef54bf to
c0f0217
Compare
|
Comment updated per review comments. |
|
@bors: r+ c0f02176a545c9587ce8c4ceba47b43d53ec2527 |
|
⌛ Testing commit c0f0217 with merge 2edb0ca... |
|
💔 Test failed - auto-win-msvc-32-opt |
There wasn't any particular reason the functions needed to be there anyway, so just get rid of them, and adjust libstd to compensate. With this change, libcore depends on exactly two floating-point functions: fmod and fmodf. They are implicitly referenced because they are used to implement "%".
eefriedman
force-pushed
the
float-dep-core
branch
from
c0f0217 to
1ddee80
Compare
|
Ugh; forgot that instcombine helpfully transforms f64 frem into f32 frem. New version stops screwing around with it. |
|
⌛ Testing commit 1ddee80 with merge 35f6d09... |
|
💔 Test failed - auto-linux-32-opt |
|
@bors: retry On Mon, Aug 17, 2015 at 2:02 PM, bors notifications@github.com wrote:
|
bors
added a commit
that referenced
this pull request
Aug 18, 2015
There wasn't any particular reason the functions needed to be there anyway, so just get rid of them, and adjust libstd to compensate. With this change, libcore depends on exactly two floating-point functions: fmod and fmodf. They are implicitly referenced because they are used to implement "%". Dependencies of libcore on Linux x86-x64 with this patch: ``` 0000000000000000 *UND* 0000000000000000 __powidf2 0000000000000000 *UND* 0000000000000000 __powisf2 0000000000000000 *UND* 0000000000000000 fmod 0000000000000000 *UND* 0000000000000000 fmodf 0000000000000000 *UND* 0000000000000000 memcmp 0000000000000000 *UND* 0000000000000000 memcpy 0000000000000000 *UND* 0000000000000000 memset 0000000000000000 *UND* 0000000000000000 rust_begin_unwind 0000000000000000 *UND* 0000000000000000 rust_eh_personality ```
bors
added a commit
that referenced
this pull request
Aug 22, 2015
These commits move libcore into a state so that it's ready for stabilization, performing some minor cleanup: * The primitive modules for integers in the standard library were all removed from the source tree as they were just straight reexports of the libcore variants. * The `core::atomic` module now lives in `core::sync::atomic`. The `core::sync` module is otherwise empty, but ripe for expansion! * The `core::prelude::v1` module was stabilized after auditing that it is a subset of the standard library's prelude plus some primitive extension traits (char, str, and slice) * Some unstable-hacks for float parsing errors were shifted around to not use the same unstable hacks (e.g. the `flt2dec` module is now used for "privacy"). After this commit, the remaining large unstable functionality specific to libcore is: * `raw`, `intrinsics`, `nonzero`, `array`, `panicking`, `simd` -- these modules are all unstable or not reexported in the standard library, so they're just remaining in the same status quo as before * `num::Float` - this extension trait for floats needs to be audited for functionality (much of that is happening in #27823) and may also want to be renamed to `FloatExt` or `F32Ext`/`F64Ext`. * Should the extension traits for primitives be stabilized in libcore? I believe other unstable pieces are not isolated to just libcore but also affect the standard library. cc #27701
This was referenced Apr 10, 2018
bors
added a commit
that referenced
this pull request
Apr 14, 2018
Add inherent methods in libcore for [T], [u8], str, f32, and f64
# Background
Primitive types are defined by the language, they don’t have a type definition like `pub struct Foo { … }` in any crate. So they don’t “belong” to any crate as far as `impl` coherence is concerned, and on principle no crate would be able to define inherent methods for them, without a trait. Since we want these types to have inherent methods anyway, the standard library (with cooperation from the compiler) bends this rule with code like [`#[lang = "u8"] impl u8 { /*…*/ }`](https://github.com/rust-lang/rust/blob/1.25.0/src/libcore/num/mod.rs#L2244-L2245). The `#[lang]` attribute is permanently-unstable and never intended to be used outside of the standard library.
Each lang item can only be defined once. Before this PR there is one impl-coherence-rule-bending lang item per primitive type (plus one for `[u8]`, which overlaps with `[T]`). And so one `impl` block each. These blocks for `str`, `[T]` and `[u8]` are in liballoc rather than libcore because *some* of the methods (like `<[T]>::to_vec(&self) -> Vec<T> where T: Clone`) need a global memory allocator which we don’t want to make a requirement in libcore. Similarly, `impl f32` and `impl f64` are in libstd because some of the methods are based on FFI calls to C’s `libm` and we want, as much as possible, libcore not to require “runtime support”.
In libcore, the methods of `str` and `[T]` that don’t allocate are made available through two **unstable traits** `StrExt` and `SliceExt` (so the traits can’t be *named* by programs on the Stable release channel) that have **stable methods** and are re-exported in the libcore prelude (so that programs on Stable can *call* these methods anyway). Non-allocating `[u8]` methods are not available in libcore: #45803. Some `f32` and `f64` methods are in an unstable `core::num::Float` trait with stable methods, but that one is **not in the libcore prelude**. (So as far as Stable programs are concerns it doesn’t exist, and I don’t know what the point was to mark these methods `#[stable]`.)
#32110 is the tracking issue for these unstable traits.
# High-level proposal
Since the standard library is already bending the rules, why not bend them *a little more*? By defining a few additional lang items, the compiler can allow the standard library to have *two* `impl` blocks (in different crates) for some primitive types.
The `StrExt` and `SliceExt` traits still exist for now so that we can bootstrap from a previous-version compiler that doesn’t have these lang items yet, but they can be removed in next release cycle. (`Float` is used internally and needs to be public for libcore unit tests, but was already `#[doc(hidden)]`.) I don’t know if #32110 should be closed by this PR, or only when the traits are entirely removed after we make a new bootstrap compiler.
# Float methods
Among the methods of the `core::num::Float` trait, three are based on LLVM intrinsics: `abs`, `signum`, and `powi`. PR #27823 “Remove dependencies on libm functions from libcore” moved a bunch of `core::num::Float` methods back to libstd, but left these three behind. However they aren’t specifically discussed in the PR thread. The `compiler_builtins` crate defines `__powisf2` and `__powidf2` functions that look like implementations of `powi`, but I couldn’t find a connection with the `llvm.powi.f32` and `llvm.powi.f32` intrinsics by grepping through LLVM’s code.
In discussion starting at #32110 (comment) Alex says that we do not want methods in libcore that require “runtime support”, but it’s not clear whether that applies to these `abs`, `signum`, or `powi`. In doubt, I’ve **removed** them for the trait and moved them to inherent methods in libstd for now. We can move them back later (or in this PR) if we decide that’s appropriate.
# Change details
For users on the Stable release channel:
* I believe this PR does not make any breaking change
* Some methods for `[u8]`, `f32`, and `f64` are newly available to `#![no_std]` users (fixes #45803)
* There should be no visible change for `std` users in terms of what programs compile or what their behavior is. (Only in compiler error messages, possibly.)
For Nightly users, additionally:
* The unstable `StrExt` and `SliceExt` traits are gone
* Their methods are now inherent methods of `str` and `[T]` (so only code that explicitly named the traits should be affected, not "normal" method calls)
* The `abs`, `signum` and `powi` methods of the `Float` trait are gone
* The `Float` trait’s unstable feature name changed to `float_internals` with no associated tracking issue, to reflect it being a permanently unstable implementation detail rather than a public API on a path to stabilization.
* Its remaining methods are now inherent methods of `f32` and `f64`.
-----
CC @rust-lang/libs for the API changes, @rust-lang/compiler for the new lang items
bors
added a commit
that referenced
this pull request
Apr 22, 2018
Add inherent methods in libcore for [T], [u8], str, f32, and f64
# Background
Primitive types are defined by the language, they don’t have a type definition like `pub struct Foo { … }` in any crate. So they don’t “belong” to any crate as far as `impl` coherence is concerned, and on principle no crate would be able to define inherent methods for them, without a trait. Since we want these types to have inherent methods anyway, the standard library (with cooperation from the compiler) bends this rule with code like [`#[lang = "u8"] impl u8 { /*…*/ }`](https://github.com/rust-lang/rust/blob/1.25.0/src/libcore/num/mod.rs#L2244-L2245). The `#[lang]` attribute is permanently-unstable and never intended to be used outside of the standard library.
Each lang item can only be defined once. Before this PR there is one impl-coherence-rule-bending lang item per primitive type (plus one for `[u8]`, which overlaps with `[T]`). And so one `impl` block each. These blocks for `str`, `[T]` and `[u8]` are in liballoc rather than libcore because *some* of the methods (like `<[T]>::to_vec(&self) -> Vec<T> where T: Clone`) need a global memory allocator which we don’t want to make a requirement in libcore. Similarly, `impl f32` and `impl f64` are in libstd because some of the methods are based on FFI calls to C’s `libm` and we want, as much as possible, libcore not to require “runtime support”.
In libcore, the methods of `str` and `[T]` that don’t allocate are made available through two **unstable traits** `StrExt` and `SliceExt` (so the traits can’t be *named* by programs on the Stable release channel) that have **stable methods** and are re-exported in the libcore prelude (so that programs on Stable can *call* these methods anyway). Non-allocating `[u8]` methods are not available in libcore: #45803. Some `f32` and `f64` methods are in an unstable `core::num::Float` trait with stable methods, but that one is **not in the libcore prelude**. (So as far as Stable programs are concerns it doesn’t exist, and I don’t know what the point was to mark these methods `#[stable]`.)
#32110 is the tracking issue for these unstable traits.
# High-level proposal
Since the standard library is already bending the rules, why not bend them *a little more*? By defining a few additional lang items, the compiler can allow the standard library to have *two* `impl` blocks (in different crates) for some primitive types.
The `StrExt` and `SliceExt` traits still exist for now so that we can bootstrap from a previous-version compiler that doesn’t have these lang items yet, but they can be removed in next release cycle. (`Float` is used internally and needs to be public for libcore unit tests, but was already `#[doc(hidden)]`.) I don’t know if #32110 should be closed by this PR, or only when the traits are entirely removed after we make a new bootstrap compiler.
# Float methods
Among the methods of the `core::num::Float` trait, three are based on LLVM intrinsics: `abs`, `signum`, and `powi`. PR #27823 “Remove dependencies on libm functions from libcore” moved a bunch of `core::num::Float` methods back to libstd, but left these three behind. However they aren’t specifically discussed in the PR thread. The `compiler_builtins` crate defines `__powisf2` and `__powidf2` functions that look like implementations of `powi`, but I couldn’t find a connection with the `llvm.powi.f32` and `llvm.powi.f32` intrinsics by grepping through LLVM’s code.
In discussion starting at #32110 (comment) Alex says that we do not want methods in libcore that require “runtime support”, but it’s not clear whether that applies to these `abs`, `signum`, or `powi`. In doubt, I’ve **removed** them for the trait and moved them to inherent methods in libstd for now. We can move them back later (or in this PR) if we decide that’s appropriate.
# Change details
For users on the Stable release channel:
* I believe this PR does not make any breaking change
* Some methods for `[u8]`, `f32`, and `f64` are newly available to `#![no_std]` users (fixes #45803)
* There should be no visible change for `std` users in terms of what programs compile or what their behavior is. (Only in compiler error messages, possibly.)
For Nightly users, additionally:
* The unstable `StrExt` and `SliceExt` traits are gone
* Their methods are now inherent methods of `str` and `[T]` (so only code that explicitly named the traits should be affected, not "normal" method calls)
* The `abs`, `signum` and `powi` methods of the `Float` trait are gone
* The `Float` trait’s unstable feature name changed to `float_internals` with no associated tracking issue, to reflect it being a permanently unstable implementation detail rather than a public API on a path to stabilization.
* Its remaining methods are now inherent methods of `f32` and `f64`.
-----
CC @rust-lang/libs for the API changes, @rust-lang/compiler for the new lang items
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There wasn't any particular reason the functions needed to be there
anyway, so just get rid of them, and adjust libstd to compensate.
With this change, libcore depends on exactly two floating-point functions:
fmod and fmodf. They are implicitly referenced because they are used to
implement "%".
Dependencies of libcore on Linux x86-x64 with this patch: