Skip to content

simd_fmin/fmax: make semantics and name consistent with scalar intrinsics#154043

Merged
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
RalfJung:simd-min-max
Mar 29, 2026
Merged

simd_fmin/fmax: make semantics and name consistent with scalar intrinsics#154043
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
RalfJung:simd-min-max

Conversation

@RalfJung
Copy link
Copy Markdown
Member

@RalfJung RalfJung commented Mar 18, 2026

This is the SIMD version of #153343: change the documented semantics of the SIMD float min/max intrinsics to that of the scalar intrinsics, and also make the name consistent. The overall semantic change this amounts to is that we restrict the non-determinism: the old semantics effectively mean "when one input is an SNaN, the result non-deterministically is a NaN or the other input"; the new semantics say that in this case the other input must be returned. For all other cases, old and new semantics are equivalent. This means all users of these intrinsics that were correct with the old semantics are still correct: the overall set of possible behaviors has become smaller, no new possible behaviors are being added.

In terms of providers of this API:

  • Miri, GCC, and cranelift already implement the new semantics, so no changes are needed.
  • LLVM is adjusted to use minimumnum nsz instead of minnum, thus giving us the new semantics.

In terms of consumers of this API:

Q: Should there be an f in the intrinsic name to indicate that it is for floats? E.g., simd_fminimum_number_nsz?

Also see #153395.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 18, 2026

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

The Miri subtree was changed

cc @rust-lang/miri

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

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

Some changes occurred to the CTFE machinery

cc @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 18, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 18, 2026

r? @chenyukang

rustbot has assigned @chenyukang.
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: compiler
  • compiler expanded to 69 candidates
  • Random selection from 15 candidates

Copy link
Copy Markdown
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

the llvm and portable-simd parts lgtm.

View changes since this review

@chenyukang
Copy link
Copy Markdown
Member

@rustbot reroll

@rustbot rustbot assigned fee1-dead and unassigned chenyukang Mar 23, 2026
@RalfJung
Copy link
Copy Markdown
Member Author

@calebzulawski it is not clear to me how to interpret that approval. Is it an "r=calebzulawski", or does it only cover some parts of this?

@antoyo @GuillaumeGomez @bjorn3 do the changes in the GCC/cranelift backend look good to you?

@folkertdev @Amanieu what about the stdarch changes?

Does anyone reading along have opinions on the name (simd_minimum_number_nsz vs simd_fminimum_number_nsz or some other suitable place for that f)?

@programmerjake
Copy link
Copy Markdown
Member

simd_minimum_number_nsz seems fine to me since it matches the scalar version

@Amanieu
Copy link
Copy Markdown
Member

Amanieu commented Mar 26, 2026

stdarch and library changes LGTM.

@antoyo
Copy link
Copy Markdown
Contributor

antoyo commented Mar 26, 2026

@antoyo @GuillaumeGomez @bjorn3 do the changes in the GCC/cranelift backend look good to you?

The GCC backend changes look good to me.

@calebzulawski
Copy link
Copy Markdown
Member

@calebzulawski it is not clear to me how to interpret that approval. Is it an "r=calebzulawski", or does it only cover some parts of this?

The whole PR looks good to me but I'm not sure I should be giving r= for this.

Does anyone reading along have opinions on the name (simd_minimum_number_nsz vs simd_fminimum_number_nsz or some other suitable place for that f)?

I don't think the f is necessary because the intrinsic is unambiguous, but adding the f there seems fine to me too.

@RalfJung
Copy link
Copy Markdown
Member Author

I think that gives us enough people to cover basically every part of this PR. (The cranelift part doesn't really change anything so no need to wait for @bjorn3 I think.)

@bors r=Amanieu,calebzulawski,antoyo

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Mar 28, 2026

📌 Commit 986a280 has been approved by Amanieu,calebzulawski,antoyo

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 28, 2026
…alebzulawski,antoyo

simd_fmin/fmax: make semantics and name consistent with scalar intrinsics

This is the SIMD version of rust-lang#153343: change the documented semantics of the SIMD float min/max intrinsics to that of the scalar intrinsics, and also make the name consistent. The overall semantic change this amounts to is that we restrict the non-determinism: the old semantics effectively mean "when one input is an SNaN, the result non-deterministically is a NaN or the other input"; the new semantics say that in this case the other input must be returned. For all other cases, old and new semantics are equivalent. This means all users of these intrinsics that were correct with the old semantics are still correct: the overall set of possible behaviors has become smaller, no new possible behaviors are being added.

In terms of providers of this API:
- Miri, GCC, and cranelift already implement the new semantics, so no changes are needed.
- LLVM is adjusted to use `minimumnum nsz` instead of `minnum`, thus giving us the new semantics.

In terms of consumers of this API:
- Portable SIMD almost certainly wants to match the scalar behavior, so this is strictly a bugfix here.
- Stdarch mostly stopped using the intrinsic, except on nvptx, where arguably the new semantics are closer to what we actually want than the old semantics (rust-lang/stdarch#2056).

Q: Should there be an `f` in the intrinsic name to indicate that it is for floats? E.g., `simd_fminimum_number_nsz`?

Also see rust-lang#153395.
rust-bors bot pushed a commit that referenced this pull request Mar 28, 2026
…uwer

Rollup of 7 pull requests

Successful merges:

 - #147811 (naked functions: respect `function-sections`)
 - #153834 (Merge `fabsf16/32/64/128` into `fabs::<F>`)
 - #154043 (simd_fmin/fmax: make semantics and name consistent with scalar intrinsics)
 - #154494 (triagebot: add reminder for bumping CI LLVM stamp)
 - #153374 (Fix LegacyKeyValueFormat report from docker build: dist-x86_64)
 - #154320 (`trim_prefix` for paths)
 - #154453 (Fix ice in rustdoc of private reexport)
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Mar 28, 2026
…alebzulawski,antoyo

simd_fmin/fmax: make semantics and name consistent with scalar intrinsics

This is the SIMD version of rust-lang#153343: change the documented semantics of the SIMD float min/max intrinsics to that of the scalar intrinsics, and also make the name consistent. The overall semantic change this amounts to is that we restrict the non-determinism: the old semantics effectively mean "when one input is an SNaN, the result non-deterministically is a NaN or the other input"; the new semantics say that in this case the other input must be returned. For all other cases, old and new semantics are equivalent. This means all users of these intrinsics that were correct with the old semantics are still correct: the overall set of possible behaviors has become smaller, no new possible behaviors are being added.

In terms of providers of this API:
- Miri, GCC, and cranelift already implement the new semantics, so no changes are needed.
- LLVM is adjusted to use `minimumnum nsz` instead of `minnum`, thus giving us the new semantics.

In terms of consumers of this API:
- Portable SIMD almost certainly wants to match the scalar behavior, so this is strictly a bugfix here.
- Stdarch mostly stopped using the intrinsic, except on nvptx, where arguably the new semantics are closer to what we actually want than the old semantics (rust-lang/stdarch#2056).

Q: Should there be an `f` in the intrinsic name to indicate that it is for floats? E.g., `simd_fminimum_number_nsz`?

Also see rust-lang#153395.
rust-bors bot pushed a commit that referenced this pull request Mar 28, 2026
…uwer

Rollup of 9 pull requests

Successful merges:

 - #150752 (Update libc to v0.2.183)
 - #153380 (stabilize new RangeFrom type and iterator)
 - #153834 (Merge `fabsf16/32/64/128` into `fabs::<F>`)
 - #154043 (simd_fmin/fmax: make semantics and name consistent with scalar intrinsics)
 - #154494 (triagebot: add reminder for bumping CI LLVM stamp)
 - #153374 (Fix LegacyKeyValueFormat report from docker build: dist-x86_64)
 - #154320 (`trim_prefix` for paths)
 - #154453 (Fix ice in rustdoc of private reexport)
 - #154515 (Notify stdarch maintainers on changes in std_detect)
rust-bors bot pushed a commit that referenced this pull request Mar 29, 2026
Rollup of 9 pull requests

Successful merges:

 - #153380 (stabilize new RangeFrom type and iterator)
 - #153834 (Merge `fabsf16/32/64/128` into `fabs::<F>`)
 - #154043 (simd_fmin/fmax: make semantics and name consistent with scalar intrinsics)
 - #154494 (triagebot: add reminder for bumping CI LLVM stamp)
 - #153374 (Fix LegacyKeyValueFormat report from docker build: dist-x86_64)
 - #154320 (`trim_prefix` for paths)
 - #154453 (Fix ice in rustdoc of private reexport)
 - #154504 (move many tests from `structs-enums` to `structs` or `enum`)
 - #154515 (Notify stdarch maintainers on changes in std_detect)
@rust-bors rust-bors bot merged commit 67ab3ac into rust-lang:main Mar 29, 2026
11 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 29, 2026
rust-timer added a commit that referenced this pull request Mar 29, 2026
Rollup merge of #154043 - RalfJung:simd-min-max, r=Amanieu,calebzulawski,antoyo

simd_fmin/fmax: make semantics and name consistent with scalar intrinsics

This is the SIMD version of #153343: change the documented semantics of the SIMD float min/max intrinsics to that of the scalar intrinsics, and also make the name consistent. The overall semantic change this amounts to is that we restrict the non-determinism: the old semantics effectively mean "when one input is an SNaN, the result non-deterministically is a NaN or the other input"; the new semantics say that in this case the other input must be returned. For all other cases, old and new semantics are equivalent. This means all users of these intrinsics that were correct with the old semantics are still correct: the overall set of possible behaviors has become smaller, no new possible behaviors are being added.

In terms of providers of this API:
- Miri, GCC, and cranelift already implement the new semantics, so no changes are needed.
- LLVM is adjusted to use `minimumnum nsz` instead of `minnum`, thus giving us the new semantics.

In terms of consumers of this API:
- Portable SIMD almost certainly wants to match the scalar behavior, so this is strictly a bugfix here.
- Stdarch mostly stopped using the intrinsic, except on nvptx, where arguably the new semantics are closer to what we actually want than the old semantics (rust-lang/stdarch#2056).

Q: Should there be an `f` in the intrinsic name to indicate that it is for floats? E.g., `simd_fminimum_number_nsz`?

Also see #153395.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 29, 2026
Rollup of 9 pull requests

Successful merges:

 - rust-lang/rust#153380 (stabilize new RangeFrom type and iterator)
 - rust-lang/rust#153834 (Merge `fabsf16/32/64/128` into `fabs::<F>`)
 - rust-lang/rust#154043 (simd_fmin/fmax: make semantics and name consistent with scalar intrinsics)
 - rust-lang/rust#154494 (triagebot: add reminder for bumping CI LLVM stamp)
 - rust-lang/rust#153374 (Fix LegacyKeyValueFormat report from docker build: dist-x86_64)
 - rust-lang/rust#154320 (`trim_prefix` for paths)
 - rust-lang/rust#154453 (Fix ice in rustdoc of private reexport)
 - rust-lang/rust#154504 (move many tests from `structs-enums` to `structs` or `enum`)
 - rust-lang/rust#154515 (Notify stdarch maintainers on changes in std_detect)
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants