clarify non-determinism docs for algebraic operations - #160496
Conversation
|
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
6ad379e to
15dd0d6
Compare
15dd0d6 to
421a5cc
Compare
|
This pull request was unapproved. |
|
I realized that the guarantee should be |
|
@bors r=Darksonn |
|
As the person that got confused, thanks :3 The |
| /// let x2 = a.algebraic_add(b); | ||
| /// assert_eq!(x1.to_bits(), x1.to_bits()); // this is guaranteed | ||
| /// # if false { | ||
| /// assert_eq!(x1, x2); // but this may fail |
There was a problem hiding this comment.
Same thing about the bits here? Even with + it could fail due to nan.
There was a problem hiding this comment.
You think that's more clear? Sure, we can do that.
421a5cc to
7b4c6fc
Compare
|
This pull request was unapproved. |
7b4c6fc to
66175a7
Compare
| /// assert_eq!(x1.to_bits(), x1.to_bits()); // this is guaranteed | ||
| /// # if false { | ||
| /// assert_eq!(x1.to_bits(), x2.to_bits()); // but this may fail | ||
| /// assert!(!x2.is_nan()); // this may also fail, even if there was no NaN input |
There was a problem hiding this comment.
I also clarified that the NaN here can arise spuriously
There was a problem hiding this comment.
It would be useful to hear in more detail how NaNs can arise spuriously. And maybe move this out of the example and to the top-level? Cause that's a pretty significant impact.
Thanks for adding more detail!
There was a problem hiding this comment.
To be more specific in why this is good to know, consider the following, with the assumption that the inputs are not NaN:
fn pairwise_sum(values: &[f64]) -> f64 {
let n = values.len();
if n > 128 {
let half = n / 2;
pairwise_sum(&values[0..half])
+ pairwise_sum(&values[half..n])
} else {
let mut total: f64 = 0.0;
for value in values {
total = total.algebraic_add(*value);
}
total
}
}My understanding is that normal float addition will never result in NaN. Can this function result in NaNs, given it's all addition and not clear how it could optimize into any other operation?
There was a problem hiding this comment.
I guess inf + -inf is NaN. But assuming all positive numbers, as in the current example.
There was a problem hiding this comment.
The docs say that no guarantee is made about the return value. That implies no guarantee about absence of NaN.
I don't actually know if anything LLVM does can cause funny NaNs here, but I think if we want to explore providing non-NaN guarantees here then that should be a new issue. Even if LLVM doesn't do this today, I think the LangRef permits them to do it in the future. This PR just clarifies the intent of what we had already written: Unsafe code must not rely on any property of the return value for soundness. In practice, we'd treat it as a bug if the code above produced a NaN as it violates the last sentence ("implementations will generally do their best to pick a reasonable tradeoff between performance and accuracy of the result"), but it would not be a soundness bug.
|
@bors r+ rollup |
clarify non-determinism docs for algebraic operations This caused some confusion [on lobsters](https://lobste.rs/s/jnznnu/faster_floating_point_math_with_rust_s_new) so it seems worth clarifying the docs a bit.
…uwer Rollup of 22 pull requests Successful merges: - #160426 (`rust-analyzer` subtree update) - #160372 (Derive the allocator used by tools from rustc's allocator) - #146882 (fully deprecate the legacy integral modules) - #158727 (std: use `readdir` on nearly all UNIX platforms) - #159727 (Various steps in moving away from the big reflection enum to reflection functions) - #160443 (normalize in relations, not generalize, when relating infer with alias) - #160457 (implement -Zllvm-target-feature) - #160480 (Single-pass ASCII lower/upper case conversion) - #160502 (Reduce number of miri tests executed on PR CI) - #157430 (std::random: use little-endian for reproducibility) - #158110 (fix macro attribute feature-gate span) - #159975 (Use real ThinVec in StmtDebugInfos) - #160001 (Suggest mutable method when iterating over binding) - #160024 (Fix mono reachability with no-op landing pads) - #160154 (Add regression test for HRTB associated type projection closure) - #160176 (No more `tests/ui/issues`!) - #160326 (Remove hidden_glob_reexports) - #160407 (Add regression tests for a number of ICEs and diagnostics issues labelled `E-needs-test`) - #160430 (bootstrap: Don't produce mutated/filtered PathSets during command-line matching) - #160472 (Minor fixes to `core::io` & `alloc::io` Documentation) - #160486 (Remove unused `FreeRegionsVisitor`) - #160496 (clarify non-determinism docs for algebraic operations) Failed merges: - #160501 (Add bootstrap CLI snapshot test for testing miri)
Rollup merge of #160496 - RalfJung:algebraic-nondet, r=Darksonn clarify non-determinism docs for algebraic operations This caused some confusion [on lobsters](https://lobste.rs/s/jnznnu/faster_floating_point_math_with_rust_s_new) so it seems worth clarifying the docs a bit.
…uwer Rollup of 22 pull requests Successful merges: - rust-lang/rust#160426 (`rust-analyzer` subtree update) - rust-lang/rust#160372 (Derive the allocator used by tools from rustc's allocator) - rust-lang/rust#146882 (fully deprecate the legacy integral modules) - rust-lang/rust#158727 (std: use `readdir` on nearly all UNIX platforms) - rust-lang/rust#159727 (Various steps in moving away from the big reflection enum to reflection functions) - rust-lang/rust#160443 (normalize in relations, not generalize, when relating infer with alias) - rust-lang/rust#160457 (implement -Zllvm-target-feature) - rust-lang/rust#160480 (Single-pass ASCII lower/upper case conversion) - rust-lang/rust#160502 (Reduce number of miri tests executed on PR CI) - rust-lang/rust#157430 (std::random: use little-endian for reproducibility) - rust-lang/rust#158110 (fix macro attribute feature-gate span) - rust-lang/rust#159975 (Use real ThinVec in StmtDebugInfos) - rust-lang/rust#160001 (Suggest mutable method when iterating over binding) - rust-lang/rust#160024 (Fix mono reachability with no-op landing pads) - rust-lang/rust#160154 (Add regression test for HRTB associated type projection closure) - rust-lang/rust#160176 (No more `tests/ui/issues`!) - rust-lang/rust#160326 (Remove hidden_glob_reexports) - rust-lang/rust#160407 (Add regression tests for a number of ICEs and diagnostics issues labelled `E-needs-test`) - rust-lang/rust#160430 (bootstrap: Don't produce mutated/filtered PathSets during command-line matching) - rust-lang/rust#160472 (Minor fixes to `core::io` & `alloc::io` Documentation) - rust-lang/rust#160486 (Remove unused `FreeRegionsVisitor`) - rust-lang/rust#160496 (clarify non-determinism docs for algebraic operations) Failed merges: - rust-lang/rust#160501 (Add bootstrap CLI snapshot test for testing miri)
This caused some confusion on lobsters so it seems worth clarifying the docs a bit.