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.

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

improve performance of signed saturating_mul #65312

Merged
merged 1 commit into from Oct 13, 2019

Conversation

@tspiteri
Copy link
Contributor

tspiteri commented Oct 11, 2019

Reciprocal throughput is improved from 2.3 to 1.7. https://godbolt.org/z/ROMiX6

Fixes #65309.

Reciprocal throughput is improved from 2.3 to 1.7.
https://godbolt.org/z/ROMiX6
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 11, 2019

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Oct 11, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 11, 2019

📌 Commit 57aae75 has been approved by nagisa

@tspiteri

This comment has been minimized.

Copy link
Contributor Author

tspiteri commented Oct 11, 2019

The code I showed in the issue is equivalent to also changing unwrap_or_else into unwrap_or, but I don't see any clear benefits there. While it does remove the branch, the reciprocal throughput remains unchanged, and I'm not sure if it can have any adverse effects with for example inlining; so I'm not changing that. https://godbolt.org/z/OqYAJ6

Copy link
Member

dtolnay left a comment

Thanks!

Reasoning through the implementation: the new code is equivalent to:

(self < 0 && rhs < 0) || (self >= 0 && rhs >= 0)

but we know self != 0 and rhs != 0 or else the checked_mul would not have overflowed.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Oct 11, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 11, 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: #64716
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 11, 2019

📌 Commit 57aae75 has been approved by dtolnay

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Oct 11, 2019

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned dtolnay Oct 11, 2019
@@ -1058,7 +1058,7 @@ $EndFeature, "
#[inline]
pub fn saturating_mul(self, rhs: Self) -> Self {
self.checked_mul(rhs).unwrap_or_else(|| {
if (self < 0 && rhs < 0) || (self > 0 && rhs > 0) {
if (self < 0) == (rhs < 0) {

This comment has been minimized.

Copy link
@RalfJung

RalfJung Oct 11, 2019

Member

This is actually also more readable than before IMO, nice :)

This comment has been minimized.

Copy link
@tspiteri

tspiteri Oct 11, 2019

Author Contributor

And in this case I think constness just has to wait :)

Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
improve performance of signed saturating_mul

Reciprocal throughput is improved from 2.3 to 1.7. https://godbolt.org/z/ROMiX6

Fixes rust-lang#65309.
bors added a commit that referenced this pull request Oct 13, 2019
Rollup of 12 pull requests

Successful merges:

 - #65214 (Split non-CAS atomic support off into target_has_atomic_load_store)
 - #65246 (vxWorks: implement get_path() and get_mode() for File fmt::Debug)
 - #65252 (expand: Simplify expansion of derives)
 - #65312 (improve performance of signed saturating_mul)
 - #65336 (Fix typo in task::Waker)
 - #65346 (nounwind tests and cleanup)
 - #65347 (Fix #[unwind(abort)] with Rust ABI)
 - #65362 (syntax: consolidate function parsing in item.rs)
 - #65366 (Implement Error::source on IntoStringError + Remove superfluous cause impls)
 - #65369 (Don't discard value names when using address or memory sanitizer)
 - #65370 (Add `dyn` to `Any` documentation)
 - #65373 (Fix typo in docs for `Rc`)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
improve performance of signed saturating_mul

Reciprocal throughput is improved from 2.3 to 1.7. https://godbolt.org/z/ROMiX6

Fixes rust-lang#65309.
bors added a commit that referenced this pull request Oct 13, 2019
Rollup of 10 pull requests

Successful merges:

 - #65214 (Split non-CAS atomic support off into target_has_atomic_load_store)
 - #65246 (vxWorks: implement get_path() and get_mode() for File fmt::Debug)
 - #65312 (improve performance of signed saturating_mul)
 - #65336 (Fix typo in task::Waker)
 - #65346 (nounwind tests and cleanup)
 - #65347 (Fix #[unwind(abort)] with Rust ABI)
 - #65366 (Implement Error::source on IntoStringError + Remove superfluous cause impls)
 - #65369 (Don't discard value names when using address or memory sanitizer)
 - #65370 (Add `dyn` to `Any` documentation)
 - #65373 (Fix typo in docs for `Rc`)

Failed merges:

r? @ghost
@bors bors merged commit 57aae75 into rust-lang:master Oct 13, 2019
4 checks passed
4 checks passed
pr Build #20191011.44 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@tspiteri tspiteri deleted the tspiteri:signed-sat-mul branch Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.