/rust

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

Use LLVM intrinsics for saturating add/sub #58003

Merged
merged 1 commit into from Jan 31, 2019

Conversation

Reviewers
No reviews
Assignees

@nagisa nagisa

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
5 participants
@nikic @nagisa @bors @rust-highfive @gnzlbg
@nikic
Copy link
Contributor

nikic commented Jan 30, 2019 •
edited

Use the [su](add|sub).sat LLVM intrinsics, if we're compiling against LLVM 8, as they should optimize and codegen better than IR based on [su](add|sub).with.overlow.

For the fallback for LLVM < 8 I'm using the same expansion that target lowering in LLVM uses, which is not the same as Rust currently uses (in particular due to the use of selects rather than branches).

Fixes #55286.
Fixes #52203.
Fixes #44500.

r? @nagisa

@rust-highfive rust-highfive assigned nagisa Jan 30, 2019

@rust-highfive rust-highfive added the S-waiting-on-review label Jan 30, 2019

@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jan 30, 2019 •
edited

The PR looks good, although I think I’d like a codegen test, especially for our own lowering. Alas we do not have max-llvm-version filter for tests :(

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 30, 2019

📌 Commit 4a4186e has been approved by nagisa

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Jan 30, 2019

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 30, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #57495
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 30, 2019

📌 Commit 4a4186e has been approved by nagisa
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jan 30, 2019

Oh, and recalling one such similar PR, we had a fairly big perf. regression there: #56009. I wonder how this will fare...
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 31, 2019

⌛️ Testing commit 4a4186e with merge fc11513...

bors added a commit that referenced this pull request Jan 31, 2019

@bors
Auto merge of #58003 - nikic:saturating-add, r=nagisa 
Use LLVM intrinsics for saturating add/sub

Use the `[su](add|sub).sat` LLVM intrinsics, if we're compiling against LLVM 8, as they should optimize and codegen better than IR based on `[su](add|sub).with.overlow`.

For the fallback for LLVM < 8 I'm using the same expansion that target lowering in LLVM uses, which is not the same as Rust currently uses (in particular due to the use of selects rather than branches).

Fixes #55286.
Fixes #52203.
Fixes #44500.

r? @nagisa
Loading status checks…
fc11513
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 31, 2019

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review and removed S-waiting-on-bors labels Jan 31, 2019

@nikic

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikic commented Jan 31, 2019

@bors retry

Failure looks spurious.

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Jan 31, 2019

@mati865 mati865 referenced this pull request Jan 31, 2019

Merged

proc_macro: make `TokenStream::from_streams` pre-allocate its vector. #58001

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 31, 2019

⌛️ Testing commit 4a4186e with merge 8a0e5fa...

bors added a commit that referenced this pull request Jan 31, 2019

@bors
Auto merge of #58003 - nikic:saturating-add, r=nagisa 
Use LLVM intrinsics for saturating add/sub

Use the `[su](add|sub).sat` LLVM intrinsics, if we're compiling against LLVM 8, as they should optimize and codegen better than IR based on `[su](add|sub).with.overlow`.

For the fallback for LLVM < 8 I'm using the same expansion that target lowering in LLVM uses, which is not the same as Rust currently uses (in particular due to the use of selects rather than branches).

Fixes #55286.
Fixes #52203.
Fixes #44500.

r? @nagisa
Loading status checks…
8a0e5fa

@pnkfelix pnkfelix referenced this pull request Jan 31, 2019

Closed

AppVeyor spuriously killing a dist- job with message "Build exceeded allowed resource quotas" #58025

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 31, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nagisa
Pushing 8a0e5fa to master...

@bors bors merged commit 4a4186e into rust-lang:master Jan 31, 2019
1 check passed

1 check passed

homu Test successful
Details
@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jan 31, 2019

📣 Toolstate changed by #58003!

Tested on commit 8a0e5fa.
Direct link to PR: #58003

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 31, 2019

@rust-highfive
📣 Toolstate changed by rust-lang/rust#58003! 
Tested on commit rust-lang/rust@8a0e5fa.
Direct link to PR: <rust-lang/rust#58003>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
Loading status checks…
e05c928

@oli-obk oli-obk referenced this pull request Jan 31, 2019

Closed

Make `saturating_add` and `saturating_sub` `const` functions #58030

@gnzlbg

This comment has been minimized.

Sign in to view
Copy link
Contributor

gnzlbg commented Feb 7, 2019

You can use llvm.sadd.sat on any integer bit width or vectors of integers.

Cool, these also should work on vector types.

@scottmcm scottmcm referenced this pull request Feb 13, 2019

Open

RFC changing the overflow behavior for usize in release builds to panic #2635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment