Skip to content

Enforce deterministic signed zero behavior in float min/max and clamp#154157

Open
anmolxlight wants to merge 1 commit intorust-lang:mainfrom
anmolxlight:feature/fix-clamp-signed-zero
Open

Enforce deterministic signed zero behavior in float min/max and clamp#154157
anmolxlight wants to merge 1 commit intorust-lang:mainfrom
anmolxlight:feature/fix-clamp-signed-zero

Conversation

@anmolxlight
Copy link

@anmolxlight anmolxlight commented Mar 20, 2026

This PR fixes #154061 by dropping the _nsz flag in minimum_number and maximum_number intrinsics.

  • Updates their fallback implementations in core::intrinsics to properly order -0.0 < +0.0 as required by the IEEE 754-2008 specification.
  • Modifies clamp in f32, f64, f16, and f128 to use self.max(min).min(max) to take advantage of the deterministic behaviour.

This guarantees deterministic output for signed zeros across all platforms, including x86 codegen.
Tested compilation locally.

This drops the `nsz` tag from `minimum_number_nsz` and `maximum_number_nsz` intrinsics,
and updates their fallback implementation in `core::intrinsics` to consistently order
`-0.0 < +0.0`. `clamp` is also rewritten to use `self.max(min).min(max)` which uses
the newly ordered min/max.

Fixes rust-lang#154061
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 20, 2026
@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[TIMING:end] tool::ToolBuild { build_compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu, tool: "tidy", path: "src/tools/tidy", mode: ToolBootstrap, source_type: InTree, extra_features: [], allow_features: "", cargo_args: [], artifact_kind: Binary } -- 13.155
[TIMING:end] tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
fmt check
Diff in /checkout/compiler/rustc_hir_analysis/src/check/intrinsic.rs:471:
         sym::minimum_number_f16 => (0, 0, vec![tcx.types.f16, tcx.types.f16], tcx.types.f16),
         sym::minimum_number_f32 => (0, 0, vec![tcx.types.f32, tcx.types.f32], tcx.types.f32),
         sym::minimum_number_f64 => (0, 0, vec![tcx.types.f64, tcx.types.f64], tcx.types.f64),
-        sym::minimum_number_f128 => {
-            (0, 0, vec![tcx.types.f128, tcx.types.f128], tcx.types.f128)
-        }
+        sym::minimum_number_f128 => (0, 0, vec![tcx.types.f128, tcx.types.f128], tcx.types.f128),
 
         sym::minimumf16 => (0, 0, vec![tcx.types.f16, tcx.types.f16], tcx.types.f16),
         sym::minimumf32 => (0, 0, vec![tcx.types.f32, tcx.types.f32], tcx.types.f32),
Diff in /checkout/compiler/rustc_hir_analysis/src/check/intrinsic.rs:483:
         sym::maximum_number_f16 => (0, 0, vec![tcx.types.f16, tcx.types.f16], tcx.types.f16),
         sym::maximum_number_f32 => (0, 0, vec![tcx.types.f32, tcx.types.f32], tcx.types.f32),
         sym::maximum_number_f64 => (0, 0, vec![tcx.types.f64, tcx.types.f64], tcx.types.f64),
-        sym::maximum_number_f128 => {
-            (0, 0, vec![tcx.types.f128, tcx.types.f128], tcx.types.f128)
-        }
+        sym::maximum_number_f128 => (0, 0, vec![tcx.types.f128, tcx.types.f128], tcx.types.f128),
 
         sym::maximumf16 => (0, 0, vec![tcx.types.f16, tcx.types.f16], tcx.types.f16),
         sym::maximumf32 => (0, 0, vec![tcx.types.f32, tcx.types.f32], tcx.types.f32),
fmt: checked 6746 files
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Build completed unsuccessfully in 0:00:51
  local time: Fri Mar 20 22:04:48 UTC 2026
  network time: Fri, 20 Mar 2026 22:04:48 GMT

@anmolxlight anmolxlight marked this pull request as ready for review March 20, 2026 22:43
@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2026

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 20, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2026

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, joboet, scottmcm

@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2026

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

@RalfJung
Copy link
Member

RalfJung commented Mar 21, 2026

This cannot land as-is since libs-api first has to agree that we even want this change. And for that we first need solid benchmarks (including some microbenchmarks) showing what the performance impact of this really is. I described all that in the issue you reference -- why did you ignore it all and not even mention it in the PR description? There's no point in doing all the work of renaming the intrinsic before we have team approval.

Doing a good evaluation of the performance impact of this is a highly non-trivial task that requires, among other things, a solid understanding of x86 assembly. I don't have that understanding, so I won't try to do this and I can't help others do it either.

I'd also recommend starting with just min and max, and worrying about clamp later.

@oli-obk oli-obk assigned oli-obk and unassigned scottmcm Mar 21, 2026
@oli-obk oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make signed zero treatment deterministic in min and max (and clamp)

6 participants