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

Bring the overflow behavior in bit shifts in sync with std #395

Merged
merged 15 commits into from
Dec 13, 2023

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Dec 5, 2023

A part of #268
Fixes #121

  • const fn bit shifts for Uint return the overflow status as CtChoice (and set the result to zero in that case, which is documented, so it's a part of the API now). Option would be better for the vartime shifts, but its methods are not const yet in stable.
  • shl/shr for BoxedUint return (Self, Choice) (not CtOption since most of its methods need the type to be ConditionallySelectable, which BoxedUint isn't). The vartime equivalents return Option<Self>.
  • operator impls panic on overflow (which is the default behavior for built-in integers)
  • made the implementations in uint/shl.rs and shr.rs more uniform and improved vartime shift performance (before it was calling a constant-time shift-by-no-more-than-limb which added some overhead)
  • improved constant-time shift performance for BoxedUint by reducing the amount of allocations
  • added an optimized BoxedUint::shl1() implementation
  • added some inlines for Limb methods which improved shift performance noticeably
  • added more benchmarks for shifts and simplify benchmark hierarchy a little (create test group directly in the respective function)
  • fixed an inefficiency in Uint shifts: we need to iterate to log2(BITS-1), not log2(BITS), because that's the maximum size of the shift.
  • Renamed sh(r/l)1_with_overflow() to sh(r/l)1_with_carry to avoid confusion - in the context of shifts we call the shift being too large an overflow.

@tarcieri
Copy link
Member

tarcieri commented Dec 5, 2023

I think these are reasonably OK things to bring in line with std

@fjarri fjarri force-pushed the shift-overflows branch 4 times, most recently from 8c67ba9 to 7300c19 Compare December 9, 2023 21:29
@fjarri fjarri marked this pull request as ready for review December 9, 2023 21:30
let nlimbs = self.nlimbs();
let mut limbs = vec![Limb::ZERO; nlimbs].into_boxed_slice();

fn shl_vartime_into(&self, dest: &mut Self, shift: u32) -> Option<()> {
Copy link
Member

Choose a reason for hiding this comment

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

As a convention I'm a fan of putting this sort of output parameter on the end, at least as far as an output buffer which is simply written into otherwise uninitialized, though I will acknowledge that's inconsistent with *_assign APIs, which operate on &mut self (and might those be applicable here?)

Copy link
Contributor Author

@fjarri fjarri Dec 9, 2023

Choose a reason for hiding this comment

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

It's not exactly assign, since it writes in a new buffer. But I don't mind changing the argument order, it's a private method anyway.

The reason it's not assign is because I wanted to optimize the cache usage, and in some cases it's impossible to do the forward iteration inplace.

@fjarri
Copy link
Contributor Author

fjarri commented Dec 9, 2023

Running the benchmarks vs master, and while most of them improved, there are some regressions in Monthomery ops with BoxedUints. Investigating now.

@fjarri
Copy link
Contributor Author

fjarri commented Dec 9, 2023

Hm, I was comparing the wrong commits. But it seems that there was a massive performance drop in "Montgomery arithmetic/multiplication, BoxedUint*BoxedUint" testcase between b169d2f and 1a0399d

@tarcieri
Copy link
Member

tarcieri commented Dec 9, 2023

@fjarri was it 2070407? That recalibrated the benchmarks using a larger input, so any of the earlier benchmarks aren't relevant, you'd have to revert the implementation and compare (but they were busted anyway, because they weren't using black_box)

@fjarri
Copy link
Contributor Author

fjarri commented Dec 9, 2023

Yep, I bisected to that point too, seems to be the reason.

src/uint/boxed/shl.rs Outdated Show resolved Hide resolved
src/uint/boxed/shl.rs Outdated Show resolved Hide resolved
src/uint/shr.rs Outdated Show resolved Hide resolved
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Looks fine now aside from the commented out test

@tarcieri tarcieri merged commit 55312b6 into RustCrypto:master Dec 13, 2023
16 checks passed
@fjarri fjarri deleted the shift-overflows branch December 13, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diverging from primitive behavior in overflowing shift
2 participants