Replace in-tree rustc_apfloat with the new version of the crate
#113843
Conversation
rustbot
added
A-meta
|
Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993). cc @eddyb These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit ef39d1b631a24f78e40286cb5f6c1e3e61b6519e with merge d724b5ab2f4ac098ff483349a80b6a3799a5177b... |
|
Would be great if you could also change Miri back to using apfloat for FMA. :) That would be basically undoing the functional part of rust-lang/miri@5a4ac1e. Then Miri tests will already check that some FMA bugs have been fixed. |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
@craterbot run start=master#d3515155216e98c23440ea92c3f49c6a0d7101fc end=try#d724b5ab2f4ac098ff483349a80b6a3799a5177b mode=build-and-test |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Finished benchmarking commit (d724b5ab2f4ac098ff483349a80b6a3799a5177b): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 658.716s -> 658.315s (-0.06%) |
@RalfJung that may be premature, given how many holes in the FMA fix I'm finding - initially I thought it was more sub-cases of the assertion failing:
But now I'm starting to suspect the FMA fix (llvm/llvm-project@e62555c) actually made things worse, introducing subtle rounding errors because of how it computes the direction of subtraction, and the original bug should be analyzed from scratch and fixed correctly this time. (I may attempt this myself once I have collected enough samples from bruteforcing Something I did miss, however, is that we don't seem to have any issue about the assertion itself? is the only issue I know related to the FMA, and is about a different behavior (related to infinities) - I only mention the assertion in comments on that issue because I hit it while checking that the issue itself was fixed. |
|
rustc_apfloat FMA is currently unused in rustc and Miri. It used to be used by Miri but I switched it to using host floats instead to fix rust-lang/miri#2468. CTFE doesn't support FMA. That's why we have no open issue for that. |
Small caveat: we're still affected by LLVM constant-folding, so we would still get the results wrong (or trigger a LLVM assertion if they're enabled) in all of those cases, just like Clang, as long as LLVM can constant-fold the FMA. |
|
🎉 Experiment
|
wesleywiser
force-pushed
the
replace_rustc_apfloat
branch
from
ab98ba6 to
9d3e35c
Compare
|
@chenyukang I took the liberty of including the test you wrote for #109573. @RalfJung I did the same for the tests you wrote for #113416 and I also added an annotation to require LLVM 16 to skip the test on older LLVM versions which have various APFloat bugs. |
|
Finished benchmarking commit (69c127efbbbba1d5118c1e803bc59c32ff6b73a0): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 650.097s -> 652.929s (0.44%) |
rustbot
removed
the
S-waiting-on-perf
wesleywiser
added
the
perf-regression-triaged
|
I don't see anything relevant to changing APFloat here, seems to perhaps be just different inlining decisions being made. I'm going to go ahead and mark as perf-regression-triaged. |
| @@ -914,16 +914,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { | |||
| let x = this.read_scalar(x)?.to_f64()?; | |||
| let exp = this.read_scalar(exp)?.to_i32()?; | |||
|
|
|||
| // Saturating cast to i16. Even those are outside the valid exponent range so | |||
There was a problem hiding this comment.
I am trying to figure out why this was originally here so that I can justify why its being removed now.
I got to this commit: 0743ed6
and now I'm stuck. Why were we clamping ldexp exponent to i16, and why don't we need to keep doing so now? I'm assuming its a weakness in the old APFloat API that has since been addressed in the new rustc_apfloat, but can you confirm that @wesleywiser ?
There was a problem hiding this comment.
FYI this is from Miri PR rust-lang/miri#902
There was a problem hiding this comment.
I'm assuming its a weakness in the old APFloat API that has since been addressed in the new rustc_apfloat, but can you confirm that @wesleywiser ?
Yes, exactly. The old APFloat API took ldexp as an i16 but the new crate takes i32 which matches what the code originally did before calling into APFloat. There's additional discussion why i16 is sufficient for this parameter in the discussion here rust-lang/miri#902 (comment).
There was a problem hiding this comment.
I see. So in theory this could have stayed as-is, but since it was an artifact of a prior limitation, we're removing it. RIght?
There was a problem hiding this comment.
Yes, I could have just added an .into() to do the i16 -> i32 conversion but since we're actually doing a i32 -> i16 -> i32 conversion, it seemed cleaner to just remove the conversions entirely.
There was a problem hiding this comment.
Indeed, thanks for the nice cleanup. :)
There was a problem hiding this comment.
I can explain this one, it's llvm/llvm-project@8606d01 [APFloat] Enlarge ExponentType to 32bit integer from late 2019 - previously, miri was stuck with an i32 and rustc_apfloat wanted i16, but they now both agree on i32.
|
@bors r+ rollup=never |
bors
added
S-waiting-on-bors
| @@ -0,0 +1,9 @@ | |||
| // run-pass | |||
| // compile-flags: -O -Zmir-opt-level=3 -Cno-prepopulate-passes | |||
| // min-llvm-version: 16.0 (requires APFloat fixes in LLVM) | |||
There was a problem hiding this comment.
It is still a mystery to me why with -Cno-prepopulate-passes this require apfloat fixes. 🤷
There was a problem hiding this comment.
Yeah, I'm not sure either. Investigating more into LLVM at this point didn't seem worthwhile to me if it's been fixed in LLVM 16 but I could be convinced otherwise 🙂
Perhaps the issue is that LLVM 14's bitcode reader fails to roundtrip the float?
There was a problem hiding this comment.
LLVM constant-folding is not a pass, but rather happens when instructions are attempted to be created.
There are passes, however, that do non-trivial simplifications, and propagate constants from e.g. loads of constant globals (though that might be in constant-folding, too? unsure), which re-triggers the constant-folding logic that first happened when the instructions were built, to account for their new inputs (you can almost think of it as "replace all uses of X with Y" simply rebuilding all instructions using X, and then the constructors of those instructions see Y now, which lets them try constant-folding again, and maybe get farther this time).
In fewer words: attempting to add a runtime LLVM Instruction to a BasicBlock may result in a Constant being produced instead (I don't know if there's a way to bypass this).
There was a problem hiding this comment.
Small addendum: you may see something different when testing calls (of either LLVM intrinsics or libm functions it knows about): IIRC, those are handled by a pass, it's only the native LLVM IR operations that get constant-folded as they're built.
|
|
||
| fn f() -> f64 { | ||
| std::hint::black_box(-1.0) % std::hint::black_box(-1.0) | ||
| } |
There was a problem hiding this comment.
This seems to be basically the same as tests/ui/const_prop/apfloat-remainder-regression.rs? Is it worth having both?
There was a problem hiding this comment.
I figured since there are two different issues, it was probably worth having two tests, but I really don't feel strongly either way.
|
Let's see if it fails on CI: rust-lang/rustc_apfloat#10 |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (0d95f91): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 649.74s -> 652.686s (0.45%) |
Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431) Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)
Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 15, in accordance with the upstream changes. * Add a patch with a backport from LLVM 17.0.3 fixing codegen for PPC, ref. rust-lang/rust#116845 Upstream changes: Version 1.73.0 (2023-10-05) ========================== Language -------- - [Uplift `clippy::fn_null_check` lint as `useless_ptr_null_checks`.] (rust-lang/rust#111717) - [Make `noop_method_call` warn by default.] (rust-lang/rust#111916) - [Support interpolated block for `try` and `async` in macros.] (rust-lang/rust#112953) - [Make `unconditional_recursion` lint detect recursive drops.] (rust-lang/rust#113902) - [Future compatibility warning for some impls being incorrectly considered not overlapping.] (rust-lang/rust#114023) - [The `invalid_reference_casting` lint is now **deny-by-default** (instead of allow-by-default)] (rust-lang/rust#112431 Compiler -------- - [Write version information in a `.comment` section like GCC/Clang.] (rust-lang/rust#97550) - [Add documentation on v0 symbol mangling.] (rust-lang/rust#97571) - [Stabilize `extern "thiscall"` and `"thiscall-unwind"` ABIs.] (rust-lang/rust#114562) - [Only check outlives goals on impl compared to trait.] (rust-lang/rust#109356) - [Infer type in irrefutable slice patterns with fixed length as array.] (rust-lang/rust#113199) - [Discard default auto trait impls if explicit ones exist.] (rust-lang/rust#113312) - Add several new tier 3 targets: - [`aarch64-unknown-teeos`] (rust-lang/rust#113480) - [`csky-unknown-linux-gnuabiv2`] (rust-lang/rust#113658) - [`riscv64-linux-android`] (rust-lang/rust#112858) - [`riscv64gc-unknown-hermit`] (rust-lang/rust#114004) - [`x86_64-unikraft-linux-musl`] (rust-lang/rust#113411) - [`x86_64-unknown-linux-ohos`] (rust-lang/rust#113061) - [Add `wasm32-wasi-preview1-threads` as a tier 2 target.] (rust-lang/rust#112922) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add `Read`, `Write` and `Seek` impls for `Arc<File>`.] (rust-lang/rust#94748) - [Merge functionality of `io::Sink` into `io::Empty`.] (rust-lang/rust#98154) - [Implement `RefUnwindSafe` for `Backtrace`] (rust-lang/rust#100455) - [Make `ExitStatus` implement `Default`] (rust-lang/rust#106425) - [`impl SliceIndex<str> for (Bound<usize>, Bound<usize>)`] (rust-lang/rust#111081) - [Change default panic handler message format.] (rust-lang/rust#112849) - [Cleaner `assert_eq!` & `assert_ne!` panic messages.] (rust-lang/rust#111071) - [Correct the (deprecated) Android `stat` struct definitions.] (rust-lang/rust#113130) Stabilized APIs --------------- - [Unsigned `{integer}::div_ceil`] (https://doc.rust-lang.org/stable/std/primitiv e.u32.html#method.div_ceil) - [Unsigned `{integer}::next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.next_multiple_of) - [Unsigned `{integer}::checked_next_multiple_of`] (https://doc.rust-lang.org/stable/std/primitive.u32.html#method.checked_next_multiple_of) - [`std::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/std/ffi/struct.FromBytesUntilNulError.html) - [`std::os::unix::fs::chown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.chown.html) - [`std::os::unix::fs::fchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.fchown.html) - [`std::os::unix::fs::lfchown`] (https://doc.rust-lang.org/stable/std/os/unix/fs/fn.lchown.html) - [`LocalKey::<Cell<T>>::get`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.get) - [`LocalKey::<Cell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set) - [`LocalKey::<Cell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take) - [`LocalKey::<Cell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace) - [`LocalKey::<RefCell<T>>::with_borrow`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow) - [`LocalKey::<RefCell<T>>::with_borrow_mut`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.with_borrow_mut) - [`LocalKey::<RefCell<T>>::set`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.set-1) - [`LocalKey::<RefCell<T>>::take`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.take-1) - [`LocalKey::<RefCell<T>>::replace`] (https://doc.rust-lang.org/stable/std/thread/struct.LocalKey.html#method.replace-1) These APIs are now stable in const contexts: - [`rc::Weak::new`] (https://doc.rust-lang.org/stable/alloc/rc/struct.Weak.html#method.new) - [`sync::Weak::new`] (https://doc.rust-lang.org/stable/alloc/sync/struct.Weak.html#method.new) - [`NonNull::as_ref`] (https://doc.rust-lang.org/stable/core/ptr/struct.NonNull.html#method.as_ref) Cargo ----- - [Encode URL params correctly for `SourceId` in `Cargo.lock`.] (rust-lang/cargo#12280) - [Bail out an error when using `cargo::` in custom build script.] (rust-lang/cargo#12332) Misc ---- Compatibility Notes ------------------- - [Update the minimum external LLVM to 15.] (rust-lang/rust#114148) - [Check for non-defining uses of return position `impl Trait`.] (rust-lang/rust#112842) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Remove LLVM pointee types, supporting only opaque pointers.] (rust-lang/rust#105545) - [Port PGO/LTO/BOLT optimized build pipeline to Rust.] (rust-lang/rust#112235) - [Replace in-tree `rustc_apfloat` with the new version of the crate.] (rust-lang/rust#113843) - [Update to LLVM 17.] (rust-lang/rust#114048) - [Add `internal_features` lint for internal unstable features.] (rust-lang/rust#108955) - [Mention style for new syntax in tracking issue template.] (rust-lang/rust#113586)
Replace the in-tree version of
rustc_apfloatwith the new version of the crate which has been correctly licensed. The new crate incorporates upstream changes from LLVM since the original port was done including many correctness fixes and has been extensively fuzz tested to validate correctness.Fixes #100233
Fixes #102403
Fixes #113407
Fixes #113409
Fixes #55993
Fixes #93224
Closes #93225
Closes #109573