Skip to content

Assertion error on arm-unknown-linux-gnueabihf for maximum/minimum float #141087

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

Open
ehuss opened this issue May 16, 2025 · 7 comments
Open

Assertion error on arm-unknown-linux-gnueabihf for maximum/minimum float #141087

ehuss opened this issue May 16, 2025 · 7 comments
Labels
A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@ehuss
Copy link
Contributor

ehuss commented May 16, 2025

I tried this code:

#![feature(float_minimum_maximum)]
fn main() {
    let x = 1.0_f64;
    let y = 2.0_f64;
    assert_eq!(x.maximum(y), y);
}

Building for target arm-unknown-linux-gnueabihf.

I expected to see this happen: Compiles

Instead, this happened: Compiler error:

rustc: /checkout/src/llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:973: void (anonymous namespace)::SelectionDAGLegalize::LegalizeOp(SDNode *): Assertion `(TLI.getTypeAction(*DAG.getContext(), Op.getValueType()) == TargetLowering::TypeLegal || Op.getOpcode() == ISD::TargetConstant || Op.getOpcode() == ISD::Register) && "Unexpected illegal type!"' failed.
Aborted

cc Tracking issue #91079, which has some discussion of whether or not this should be implemented using an intrinsic, which I was a little unclear about or whether this limitation is known.

Meta

rustc --version --verbose:

rustc 1.89.0-nightly (414482f6a 2025-05-13)
binary: rustc
commit-hash: 414482f6a0d4e7290f614300581a0b55442552a3
commit-date: 2025-05-13
host: x86_64-unknown-linux-gnu
release: 1.89.0-nightly
LLVM version: 20.1.4
@ehuss ehuss added C-bug Category: This is a bug. A-floating-point Area: Floating point numbers and arithmetic labels May 16, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 16, 2025
@jieyouxu jieyouxu added O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 16, 2025
@ehuss
Copy link
Contributor Author

ehuss commented May 17, 2025

cc @Urgau or @tgross35. Not sure if either of you would know what's going on with this. If it doesn't look like there is an easy fix, I will need to disable these tests before the next beta branch/bootstrap bump (which I think is about a month from now) since the next release will start testing these via doctests.

@tgross35
Copy link
Contributor

tgross35 commented May 17, 2025

Any chance you can bisect this? I assume it was for the recent (~1-2weeks) change to use an intrinsic for these (edit: this one, bisection probably isn't needed #140792)

@tgross35
Copy link
Contributor

If we need to fix the behavior, it would be a bit less churn to delete the connection between rustc's intrinsic and LLVM's (so the fallback gets used) rather than reverting the entire thing.

But we definitely should be using intrinsics for these; since they are unstable, I'm tempted to say we should disable the failing tests only on relevant platforms rather than switching back from intrinsics. At least that would give us a better chance of catching other failing platforms before stabilization, rather than changing to intrinsics and discovering more problems after stabilization.

@ehuss
Copy link
Contributor Author

ehuss commented May 17, 2025

Oh, I didn't expect this to be a new behavior. Yea, bisected to #140912 which I assume was #140792.

Yea, my plan is to just disable it for that one platform. There's no big rush, I was just curious if there was a simple fix. We still have plenty of time. If there isn't anything ready in a while, I'll post the following patch.

diff --git a/library/core/src/num/f64.rs b/library/core/src/num/f64.rs
index 81ab0f14c2b..0aef4a94ed7 100644
--- a/library/core/src/num/f64.rs
+++ b/library/core/src/num/f64.rs
@@ -943,7 +943,7 @@ pub const fn min(self, other: f64) -> f64 {
     /// This returns NaN when *either* argument is NaN, as opposed to
     /// [`f64::max`] which only returns NaN when *both* arguments are NaN.
     ///
-    /// ```
+    /// ```ignore-arm-unknown-linux-gnueabihf (see https://github.com/rust-lang/rust/issues/141087)
     /// #![feature(float_minimum_maximum)]
     /// let x = 1.0_f64;
     /// let y = 2.0_f64;
@@ -970,7 +970,7 @@ pub const fn maximum(self, other: f64) -> f64 {
     /// This returns NaN when *either* argument is NaN, as opposed to
     /// [`f64::min`] which only returns NaN when *both* arguments are NaN.
     ///
-    /// ```
+    /// ```ignore-arm-unknown-linux-gnueabihf (see https://github.com/rust-lang/rust/issues/141087)
     /// #![feature(float_minimum_maximum)]
     /// let x = 1.0_f64;
     /// let y = 2.0_f64;

@tgross35
Copy link
Contributor

tgross35 commented May 17, 2025

@Urgau opened llvm/llvm-project#139380 and llvm/llvm-project#139381 so you could probably use that as a template to open an issue upstream. But it would take a while for that fix to bubble up to us, so the patch LGTM if you wind up posting it, assuming the other float types are somehow not broken on that platform.

@Urgau
Copy link
Member

Urgau commented May 18, 2025

Opened llvm/llvm-project#140445 about it.

@Urgau
Copy link
Member

Urgau commented May 19, 2025

Started a thread on Zulip about the issues with those LLVM intrinsics: #t-compiler/llvm > `llvm.minimum`/`llvm.maximum` issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants