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.

Sign up for GitHub

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

Jump to bottom

intrinsics::simd: add missing functions, avoid UB-triggering fast-math #121223

Merged
merged 3 commits into from
Feb 22, 2024
Merged

intrinsics::simd: add missing functions, avoid UB-triggering fast-math #121223

merged 3 commits into from
Feb 22, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 17, 2024 •
edited
Loading

Turns out stdarch declares a bunch more SIMD intrinsics that are still missing from libcore.
I hope I got the docs and in particular the safety requirements right for these "unordered" and "nanless" intrinsics.

Many of these are unused even in stdarch, but they are implemented in the codegen backend, so we may as well list them here.

r? @Amanieu
Cc @calebzulawski @workingjubilee

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2024

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

@workingjubilee
Copy link
Member

...I knew about _insert and _extract but the rest frighten me.

@RalfJung
Copy link
Member Author

Most of them are unused. Should we remove those from codegen instead of adding them here?

@workingjubilee
Copy link
Member

simd_reduce_add_unordered, simd_reduce_mul_unordered, simd_reduce_min_nanless, and simd_reduce_max_nanless should probably go, yeah.

@RalfJung
Copy link
Member Author

The first two are used though: rust-lang/stdarch#1533.

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

Ah, this is blocked on rust-lang/stdarch#1529 propagating to this repo.

@GuillaumeGomez
Copy link
Member

Waiting for #121185. :)

@RalfJung
Copy link
Member Author

I don't think that has my recent patches included.

calebzulawski
calebzulawski reviewed Feb 18, 2024
View reviewed changes
library/core/src/intrinsics/simd.rs Outdated
///
/// # Safety
///
/// All input elements must be finite (i.e., not NAN and not +/- INF).
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment accurate? I assume there's a std::arch reason but I wouldn't have picked this intrinsic name

Copy link
Member

Choose a reason for hiding this comment

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

This definitely shouldn't be using UB-causing fast-math flags without clearly having _fast in the name. Can we fix this intrinsic to only use the reassoc flag instead of the fast flag? This will eliminate the potential for UB.

Copy link
Member

Choose a reason for hiding this comment

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

This will also fix rust-lang/stdarch#1533.

Copy link
Member Author

@RalfJung RalfJung Feb 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The comment is accurate wrt the current implementation in the LLVM codegen backend.

Let's use rust-lang/stdarch#1533 to discuss alternative implementations.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Amanieu
Copy link
Member

Amanieu commented Feb 19, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2024

📌 Commit f70538c has been approved by Amanieu

It is now in the queue for this repository.

@bors bors 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 Feb 19, 2024
Noratrieb added a commit to Noratrieb/rust that referenced this pull request Feb 20, 2024
@Noratrieb
Rollup merge of rust-lang#121223 - RalfJung:simd-intrinsics, r=Amanieu
37046c3 
intrinsics::simd: add missing functions

Turns out stdarch declares a bunch more SIMD intrinsics that are still missing from libcore.
I hope I got the docs and in particular the safety requirements right for these "unordered" and "nanless" intrinsics.

Many of these are unused even in stdarch, but they are implemented in the codegen backend, so we may as well list them here.

r? `@Amanieu`
Cc `@calebzulawski` `@workingjubilee`
@Noratrieb Noratrieb mentioned this pull request Feb 20, 2024
Closed
@RalfJung RalfJung force-pushed the simd-intrinsics branch 2 times, most recently from 2ab96fe to f70538c Compare February 20, 2024 10:10
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
@bors
Auto merge of rust-lang#121332 - Nilstrieb:rollup-qwmc1i4, r=Nilstrieb
43b954a 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#121167 (resolve: Scale back unloading of speculatively loaded crates)
 - rust-lang#121196 (Always inline check in `assert_unsafe_precondition` with cfg(debug_assertions))
 - rust-lang#121206 (Top level error handling)
 - rust-lang#121223 (intrinsics::simd: add missing functions)
 - rust-lang#121241 (Implement `NonZero` traits generically.)
 - rust-lang#121242 (Generate `getelementptr` instead of `inttoptr` for `ptr::invalid`)
 - rust-lang#121278 (Remove the "codegen" profile from bootstrap)
 - rust-lang#121286 (Rename `ConstPropLint` to `KnownPanicsLint`)

r? `@ghost`
`@rustbot` modify labels: rollup
@RalfJung
Copy link
Member Author

@bors r-
I added a commit that removes the nanless min/max intrinsics. @Amanieu is that fine for you?

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2024
@RalfJung
Copy link
Member Author

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Feb 21, 2024

📌 Commit 2df74bb has been approved by Amanieu

It is now in the queue for this repository.

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 21, 2024
RalfJung
RalfJung commented Feb 21, 2024
View reviewed changes
compiler/rustc_codegen_llvm/src/builder.rs
unsafe {
let instr = llvm::LLVMRustBuildVectorReduceFAdd(self.llbuilder, acc, src);
llvm::LLVMRustSetAlgebraicMath(instr);
Copy link
Member Author

Choose a reason for hiding this comment

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

@saethlin you changed these to "algebraic", but I think they should be just "reassoc" to match what clang does. (And even that is questionable, see rust-lang/stdarch#1533. But that's a separate issue.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe the "algebraic" implementation is technically wrong. But it was an improvement to land because it doesn't explode so dramatically :p

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Feb 21, 2024

📌 Commit 64efe80 has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 21, 2024

⌛ Testing commit 64efe80 with merge 975c7bf...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
@bors
Auto merge of rust-lang#121223 - RalfJung:simd-intrinsics, r=Amanieu
975c7bf 
intrinsics::simd: add missing functions, avoid UB-triggering fast-math

Turns out stdarch declares a bunch more SIMD intrinsics that are still missing from libcore.
I hope I got the docs and in particular the safety requirements right for these "unordered" and "nanless" intrinsics.

Many of these are unused even in stdarch, but they are implemented in the codegen backend, so we may as well list them here.

r? `@Amanieu`
Cc `@calebzulawski` `@workingjubilee`
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 21, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 21, 2024
@RalfJung
Copy link
Member Author

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Feb 21, 2024

📌 Commit 07b6240 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors 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 Feb 21, 2024
@bors
Copy link
Contributor

bors commented Feb 22, 2024

⌛ Testing commit 07b6240 with merge c1b478e...

@bors
Copy link
Contributor

bors commented Feb 22, 2024

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing c1b478e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 22, 2024
@bors bors merged commit c1b478e into rust-lang:master Feb 22, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c1b478e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.2% [0.7%, 4.5%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.5% [2.3%, 2.7%] 2
Regressions ❌
(secondary)		 3.1% [1.0%, 6.1%] 6
Improvements ✅
(primary)		 -1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.2% [-1.4%, 2.7%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-2.3%, -1.8%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.169s -> 649.414s (-0.12%)
Artifact size: 310.96 MiB -> 310.96 MiB (-0.00%)

@RalfJung RalfJung deleted the simd-intrinsics branch February 24, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saethlin saethlin saethlin left review comments

@calebzulawski calebzulawski calebzulawski approved these changes

@Amanieu Amanieu Amanieu left review comments

Assignees

@Amanieu Amanieu

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
10 participants
@RalfJung @rustbot @workingjubilee @rust-log-analyzer @GuillaumeGomez @Amanieu @bors @rust-timer @calebzulawski @saethlin