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

core: Fix overflow in int::mod_euc when self < 0 && rhs == MIN #50185

Merged
merged 2 commits into from
Apr 24, 2018

Conversation

tesaguri
Copy link
Contributor

This commit removes usage of abs, which overflows when self == MIN.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2018
@@ -1765,7 +1765,11 @@ assert_eq!((-a).mod_euc(-b), 1);
pub fn mod_euc(self, rhs: Self) -> Self {
let r = self % rhs;
if r < 0 {
r + rhs.abs()
if rhs.is_negative() {
Copy link
Member

Choose a reason for hiding this comment

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

This line and the previous might as well be consistent (whichever of < 0 or is_negative you prefer).

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

This was my fault for not paying close enough attention during review! Thanks for catching it!

@kennytm
Copy link
Member

kennytm commented Apr 24, 2018

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 24, 2018

📌 Commit 104c64d has been approved by kennytm

@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 Apr 24, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 24, 2018
core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`

This commit removes usage of `abs`, which overflows when `self == MIN`.
bors added a commit that referenced this pull request Apr 24, 2018
Rollup of 11 pull requests

Successful merges:

 - #49461 (std: Child::kill() returns error if process has already exited)
 - #49727 (Add Cell::update)
 - #49812 (Fix revision support for UI tests.)
 - #49829 (Add doc links to `std::os` extension traits)
 - #49906 (Stabilize `std::hint::unreachable_unchecked`.)
 - #49970 (Deprecate Read::chars and char::decode_utf8)
 - #49985 (don't see issue #0)
 - #50118 (fix search bar bug)
 - #50139 (encourage descriptive issue titles)
 - #50174 (Use FxHashMap in syntax_pos::symbol::Interner::intern.)
 - #50185 (core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`)

Failed merges:
@bors
Copy link
Contributor

bors commented Apr 24, 2018

☔ The latest upstream changes (presumably #50191) made this pull request unmergeable. Please resolve the merge conflicts.

@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 Apr 24, 2018
@bors bors merged commit 104c64d into rust-lang:master Apr 24, 2018
@tesaguri tesaguri deleted the mod_euc-fix-overflow branch April 24, 2018 11:16
@Boscop
Copy link

Boscop commented May 3, 2018

I'd prefer mod_euc to have no branches, because it often occurs in very hot loops in my code.
Can we change it to remove the branches? So instead of this:

pub fn mod_euc(self, rhs: Self) -> Self {
	let r = self % rhs;
	if r < 0 {
		if rhs < 0 {
			r - rhs
		} else {
			r + rhs
		}
	} else {
		r
	}
}

Something like this:

pub fn mod_euc(self, rhs: Self) -> Self {
	let r = self % rhs;
	let r_pos = [r + rhs, r - rhs][(rhs < 0) as usize];
	[r, r_pos][(r < 0) as usize)]
}

The compiler should be able to remove the array bounds checks, right?

@varkor
Copy link
Member

varkor commented May 3, 2018

@Boscop: I'm curious to know how differently this performs — have you written a benchmark to see? (Also, maybe it would be better to continue this on the tracking issue where it's more discoverable!)

@Boscop
Copy link

Boscop commented May 4, 2018

@varkor
Nvm, it seems that the current version is the fastest (tested on x64 Win 8.1 with i64 ints only):

test tests::bench_mod_euc_branching    ... bench:  33,072,518 ns/iter (+/- 1,355,002)
test tests::bench_mod_euc_branchless   ... bench:  34,965,845 ns/iter (+/- 1,265,057)
test tests::bench_mod_euc_branchless_2 ... bench:  35,493,871 ns/iter (+/- 838,259)
test tests::bench_mod_euc_branchless_u ... bench:  35,033,348 ns/iter (+/- 1,717,562)

Is this the correct way to write a benchmark?

#[cfg(test)]
mod tests {
	use test::{Bencher, black_box};

	pub fn mod_euc_branching(slf: i64, rhs: i64) -> i64 {
		let r = slf % rhs;
		if r < 0 {
			if rhs < 0 {
				r - rhs
			} else {
				r + rhs
			}
		} else {
			r
		}
	}

	pub fn mod_euc_branchless(slf: i64, rhs: i64) -> i64 {
		let r = slf % rhs;
		let r_pos = [r + rhs, r - rhs][(rhs < 0) as usize];
		[r, r_pos][(r < 0) as usize]
	}

	pub fn mod_euc_branchless_u(slf: i64, rhs: i64) -> i64 {
		let r = slf % rhs;
		unsafe {
			let r_pos = *[r + rhs, r - rhs].get_unchecked((rhs < 0) as usize);
			*[r, r_pos].get_unchecked((r < 0) as usize)
		}
	}

	pub fn mod_euc_branchless_2(slf: i64, rhs: i64) -> i64 {
		let r = slf % rhs;
		let r_pos = r + rhs - 2 * rhs * (rhs < 0) as i64;
		r + (r_pos - r) * (r < 0) as i64
	}

	#[bench]
	fn bench_mod_euc_branching(b: &mut Bencher) {
		b.iter(|| {
			for a in -1000..1000 {
				for b in -1000..1000 {
					if b == 0 { continue; }
					black_box(mod_euc_branching(a, b));
				}
			}
		});
	}

	#[bench]
	fn bench_mod_euc_branchless(b: &mut Bencher) {
		b.iter(|| {
			for a in -1000..1000 {
				for b in -1000..1000 {
					if b == 0 { continue; }
					black_box(mod_euc_branchless(a, b));
				}
			}
		});
	}

	#[bench]
	fn bench_mod_euc_branchless_u(b: &mut Bencher) {
		b.iter(|| {
			for a in -1000..1000 {
				for b in -1000..1000 {
					if b == 0 { continue; }
					black_box(mod_euc_branchless_u(a, b));
				}
			}
		});
	}

	#[bench]
	fn bench_mod_euc_branchless_2(b: &mut Bencher) {
		b.iter(|| {
			for a in -1000..1000 {
				for b in -1000..1000 {
					if b == 0 { continue; }
					black_box(mod_euc_branchless_2(a, b));
				}
			}
		});
	}
}

@varkor
Copy link
Member

varkor commented May 4, 2018

@Boscop: yeah, that looks fine to me. Thanks for checking it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants