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

Fix incorrect rounding with subnormal/zero results of float multiplication #637

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

beetrees
Copy link
Contributor

Fix float multiplication that results in subnormals (or zero), and enable tests for those cases. Also remove dead code left over from when this was ported from compiler-rt (the other branches of the if statement are unreachable as shift is always less than bits at this point). The equivalent code in compiler-rt does not have this bug.

Fixes #616

@tgross35
Copy link
Contributor

There has to be a bug in either GCC or LLVM's libraries at least on some platforms for f128, I was able to reproduce in C llvm/llvm-project#91840.

The clippy failure is from #634 (comment)

@beetrees beetrees force-pushed the fix-float-mul branch 2 times, most recently from 01a7c0c to 01b0b37 Compare June 30, 2024 22:02
@tgross35
Copy link
Contributor

Probably just #![allow(out_of_scope_macro_calls)] to fix the rest until rust-lang/rust#126987 gets merged

@beetrees
Copy link
Contributor Author

Allowing the lint results in the contradictory

 error: unknown lint: `out_of_scope_macro_calls`
  --> src/probestack.rs:52:10
   |
52 | #![allow(out_of_scope_macro_calls)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unknown-lints` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unknown_lints)]`

error: cannot find macro `define_rust_probestack` in this scope
   --> src/probestack.rs:137:5
    |
137 |     define_rust_probestack!(
    |     ^^^^^^^^^^^^^^^^^^^^^^
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #124535 <https://github.com/rust-lang/rust/issues/124535>
    = help: import `macro_rules` with `use` to make it callable above its definition
    = note: `-D out-of-scope-macro-calls` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(out_of_scope_macro_calls)]`

so I've added a temporary use define_rust_probestack; instead.

@beetrees
Copy link
Contributor Author

CI is now green 🎉. Thanks for the help.

Comment on lines -20 to +23
// FIXME(f16_f128): rounding error
// FIXME(#616): re-enable once fix is in nightly
// <https://github.com/rust-lang/compiler-builtins/issues/616>
"mul_f128",
"mul_f32",
"mul_f64",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why will these be fixed with the nightly bump? I guess if __mul[sd]f3 don't exist since they are expected to lower to assembly, then linking the extern "C" symbols is linking to the weak ones from this library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your guess is correct.

Comment on lines 36 to 39
// FIXME(f16_f128): Incorrect rounding.
// <https://github.com/llvm/llvm-project/issues/91840>
const AARCH64_SKIPPED: &[&str] = &["mul_f128"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this must have been reproduced in CI then, which is good. Any idea where the problem is coming from? https://github.com/llvm/llvm-project/blob/8598bcb9934dca16ea16d87304e00defc85d986c/compiler-rt/lib/builtins/fp_lib.h and https://github.com/llvm/llvm-project/blob/8598bcb9934dca16ea16d87304e00defc85d986c/compiler-rt/lib/builtins/fp_mul_impl.inc don't seem to have any architecture-specific behavior except for fmaxf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just reproduced the bug using the instructions from llvm/llvm-project#91840 on x86_64-unknown-linux-gnu; I don't think the bug is platform specific, just compiler-rt specific. It only shows up on AArch64 in CI as that's the only architecture tested in compiler-builtins CI that used to build the C version of the builtin in the version of compiler-builtins currently used by nightly rustc (in the build.rs that's currently used by nightly, notice how multf3.c is only included on aarch64/arm64ec, mips64 and loongarch64).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the FIXME.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great, guess I just didn't test this on other platforms. Thanks for testing

@Amanieu Amanieu merged commit c1c8fcc into rust-lang:master Jul 5, 2024
24 checks passed
@beetrees beetrees deleted the fix-float-mul branch July 5, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rounding error in __mulsf3
3 participants