-
Notifications
You must be signed in to change notification settings - Fork 242
Absorb the libm
repository into compiler-builtins
.
#822
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
Merged
Conversation
This file contains hidden or 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
It is currently getting the default of 1 or 2. Since this operation should always be infinite precision, no deviation is allowed.
Currently `logspace` does a lossy cast from `F::Int` to `usize`. This could be problematic in the rare cases that this is called with a step count exceeding what is representable in `usize`. Resolve this by instead adding bounds so the float's integer type itself can be iterated.
We want to be able to adjust our configuration based on whether we are running in CI, propagate this so our tests can use it.
Rather than collecting a list of file names in `libm-test/build.rs`, just use a script to parse rustdoc's JSON output.
Now that we are using rustdoc output to locate public functions, the test is indicating a few that were missed since they don't have their own function. Update everything to now include the following routines: * `erfc` * `erfcf` * `y0` * `y0f` * `y1` * `y1f` * `yn` * `ynf`
Use `rustdoc` JSON for API list, add functions that were missing
Once we start addinf `f16` and `f128` routines, we will need to have this cfg for almost all uses of `for_each_function`. Rather than needing to specify this each time, always emit `#[cfg(f16_enabled)]` or `#[cfg(f128_enabled)]` for each function that uses `f16` or `f128`, respectively.
Using the same name as the routines themselves means this will correctly get picked up by the CI job looking for exhaustive tests.
Currently our implementations for `abs` and `copysign` are defined on the trait, and these are then called from `generic`. It would be better to call core's `.abs()` / `.copysign(y)`, but we can't do this in the generic because calling the standalone function could become recursive (`fabsf` becomes `intrinsics::fabsf32`, that may lower to a call to `fabsf`). Change this so the traits uses the call to `core` if available, falling back to a call to the standalone generic function. In practice the recursion isn't likely to be a problem since LLVM probably always lowers `abs`/`copysign` to assembly, but this pattern should be more correct for functions that we will add in the future (e.g. `fma`). This should eventually be followed by a change to call the trait methods rather than `fabs`/`copysign` directly.
Make it more obvious what the expected ULP for a given routine is. This also narrows ULP to 0 for operations that require exact results.
This is a nonfunctional change.
There isn't any need to cache the integer since it gets provided as an argument anyway. Simplify this in `jn` and `yn`.
Rug provides `trunc_fract_round`, which implements `modf`, use it to add a test.
This implementation comes from `rug::Float::to_f32_exp` [1]. [1]: https://docs.rs/rug/1.26.1/rug/struct.Float.html#method.to_f32_exp
Occasionally it is useful to see some information from running tests without making everything noisy from `--nocapture`. Add a function to log this kind of output to a file, and print the file as part of CI.
Currently, tests use a handful of constants to determine how many iterations to perform: `NTESTS`, `AROUND`, and `MAX_CHECK_POINTS`. This configuration is not very straightforward to adjust and needs to be repeated everywhere it is used. Replace this with new functions in the `run_cfg` module that determine iteration counts in a more reusable and documented way. This only updates `edge_cases` and `domain_logspace`, `random` is refactored in a later commit.
Introduce the `KnownSize` iterator wrapper, which allows providing the size at construction time. This provides an `ExactSizeIterator` implemenation so we can check a generator's value count during testing.
Currently, all inputs are generated and then cached. This works reasonably well but it isn't very configurable or extensible (adding `f16` and `f128` is awkward). Replace this with a trait for generating random sequences of tuples. This also removes possible storage limitations of caching all inputs.
Streamline the way that test iteration count is determined (replace `NTESTS`)
The `support` module that this feature makes public will be useful for implementations in `compiler-builtins`, not only for testing. Give this feature a more accurate name.
New random seeds seem to indicate that this test does have some more failures, this is a recent failure on i586: ---- musl_random_jnf stdout ---- Random Musl jnf arg 1/2: 100 iterations (10000 total) using `LIBM_SEED=nLfzQ3U1OBVvqWaMBcto84UTMsC5FIaC` Random Musl jnf arg 2/2: 100 iterations (10000 total) using `LIBM_SEED=nLfzQ3U1OBVvqWaMBcto84UTMsC5FIaC` thread 'musl_random_jnf' panicked at crates/libm-test/tests/compare_built_musl.rs:43:51: called `Result::unwrap()` on an `Err` value: input: (205, 5497.891) (0x000000cd, 0x45abcf21) expected: 7.3291517e-6 0x36f5ecef actual: 7.331668e-6 0x36f6028c Caused by: ulp 5533 > 4000 It seems unlikely that `jn` would somehow have better precision than `j0`/`j1`, so just use the same precision.
Update the script to produce, in addition to the simple text list, a JSON file listing routine names, the types they work with, and the source files that contain a function with the routine name. This gets consumed by another script and will be used to determine which extensive CI jobs to run.
Add a generator that will test all inputs for input spaces `u32::MAX` or smaller (e.g. single-argument `f32` routines). For anything larger, still run approximately `u32::MAX` tests, but distribute inputs evenly across the function domain. Since we often only want to run one of these tests at a time, this implementation parallelizes within each test using `rayon`. A custom test runner is used so a progress bar is possible. Specific tests must be enabled by setting the `LIBM_EXTENSIVE_TESTS` environment variable, e.g. LIBM_EXTENSIVE_TESTS=all_f16,cos,cosf cargo run ... Testing on a recent machine, most tests take about two minutes or less. The Bessel functions are quite slow and take closer to 10 minutes, and FMA is increased to run for about the same.
Add a CI job with a dynamically calculated matrix that runs extensive jobs on changed files. This makes use of the new `function-definitions.json` file to determine which changed files require full tests for a routine to run.
Add exhaustive/extensive tests
Since `fmod` is generic, there isn't any need to have the small wrappers in separate files. Most operations was done in [1] but `fmod` was omitted until now. [1]: rust-lang/libm#537
Benchmarks for [1] seemed to indicate that repository organization for some reason had an effect on performance, even though the exact same rustc commands were running (though some with a different order). After investigating more, it appears that dependencies may have an affect on inlining thresholds for generic functions. It is surprising that this happens, we more or less expect that public functions will be standalone but everything they call will be inlined. To help ensure this, mark all generic functions `#[inline]` if they should be merged into the public function. Zulip discussion at [2]. [1]: rust-lang/libm#533 [2]: https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/Dependencies.20affecting.20codegen/with/513079387
In preparation for switching to a virtual manifest, move the `libm` crate into a subdirectory and update paths to match. Updating `Cargo.toml` is done in the next commit so git tracks the moved file correctly.
Move the workspace configuration to a virtual manifest. This reorganization makes a more clear separation between package contents and support files that don't get distributed. It will also make it easier to merge this repository with `compiler-builtins` which is planned (builtins had a similar update done in [1]). LICENSE.txt and README.md are symlinkedinto the new directory to ensure they get included in the package. [1]: #702
Unfortunately this means we lose use of the convenient name `gen`, so this includes a handful of renaming. We can't increase the edition for `libm` yet due to MSRV, but we can enable `unsafe_op_in_unsafe_fn` to help make that change smoother in the future.
GH is only showing 250 commits, the relevant merge is a0db2ca. |
compiler-builtins
.libm
repository into compiler-builtins
.
Amanieu
approved these changes
Apr 19, 2025
…orb-libm Absorb the libm repository into `compiler-builtins`. This was done using `git-filter-repo` to ensure hashes mentioned in commit messages were correctly rewritten, I used the same strategy to merge `ctest` into `libc` [1] and it worked quite well. Approximately: # `git filter-repo` requires a clean clone git clone https://github.com/rust-lang/libm.git # Move all code to a `libm` subdirectory for all history git filter-repo --to-subdirectory-filter libm # The default merge messages are "merge pull request #nnn from # user/branch". GH links these incorrectly in the new repo, so # rewrite messages from `#nnn` to `rust-lang/libm#nnn`. echo 'regex:(^|\s)(#\d+)==>\1rust-lang/libm\2' > messages.txt git filter-repo --replace-message messages.txt # Re-add a remote and push as a new branch git remote add upstream https://github.com/rust-lang/libm.git git switch -c merge-into-builtins-prep git push --set-upstream upstream merge-into-builtins-prep # Now in a compiler-builtins, add `libm` as a remote git remote add libm https://github.com/rust-lang/libm.git git fetch libm # Do the merge that creates this commit git merge libm/merge-into-builtins-prep --allow-unrelated-histories The result should be correct git history and blame for all files, with messages that use correct rewritten hashes when they are referenced. There is some reorganization and CI work needed, but that will be a follow up. After this merges I will need to push tags from `libm`, which I have already rewritten to include a `libm-` prefix. Old tags in compiler-builtins should likely also be rewritten to add a prefix (we already have this for newer tags), but this can be done at any point. * Original remote: https://github.com/rust-lang/libm.git * Default HEAD: c94017af75c3ec4616d5b7f9b6b1b3826b934469 ("Migrate all crates except `libm` to edition 2024") * HEAD after rewriting history: 15fb630 ("Migrate all crates except `libm` to edition 2024") [1]: rust-lang/libc#4283 (comment)
946e467 is now the relevant merge commit |
I asked Mark to fastforward master so GH doesn't interfere with history, everything looks good. |
Merged
tgross35
added a commit
to tgross35/rust-libm
that referenced
this pull request
Apr 20, 2025
Since [1], `libm` is now part of the `compiler-builtins` repositoy, so this repo will be archived. Update the README to reflect that. [1]: rust-lang/compiler-builtins#822
tgross35
added a commit
to rust-lang/libm
that referenced
this pull request
Apr 20, 2025
Since [1], `libm` is now part of the `compiler-builtins` repositoy, so this repo will be archived. Update the README to reflect that. [1]: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 21, 2025
This has been merged into https://github.com/rust-lang/compiler-builtins. Link: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 21, 2025
Mention libm in the description, and remove the homepage that points to a long-closed issue. Also update quotes to match most other files. Link: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 22, 2025
This has been merged into https://github.com/rust-lang/compiler-builtins. Link: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 22, 2025
Mention libm in the description, and remove the homepage that points to a long-closed issue. Also update quotes to match most other files. Link: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 22, 2025
This has been merged into https://github.com/rust-lang/compiler-builtins. Link: rust-lang/compiler-builtins#822
tgross35
added a commit
to tgross35/rust-team
that referenced
this pull request
Apr 22, 2025
Mention libm in the description, and remove the homepage that points to a long-closed issue. Also update quotes to match most other files. Link: rust-lang/compiler-builtins#822
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.
Merge remote-tracking branch 'libm/merge-into-builtins-prep' into absorb-libm
Absorb the libm repository into
compiler-builtins
.This was done using
git-filter-repo
to ensure hashes mentioned incommit messages were correctly rewritten, I used the same strategy to
merge
ctest
intolibc
1 and it worked quite well. Approximately:The result should be correct git history and blame for all files, with
messages that use correct rewritten hashes when they are referenced.
There is some reorganization and CI work needed, but that will be a
follow up.
After this merges I will need to push tags from
libm
, which I havealready rewritten to include a
libm-
prefix. Old tags incompiler-builtins should likely also be rewritten to add a prefix (we
already have this for newer tags), but this can be done at any point.
crates except
libm
to edition 2024")("Migrate all crates except
libm
to edition 2024")