[stdlib] Implement efficient DoubleWidth division and fix division-related bugs #14219
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xwu
commented
Jan 28, 2018
|
|
||
| if isNegative { | ||
| let (lowComplement, overflow) = (~low).addingReportingOverflow(1) | ||
| return (~high + (overflow ? 1 : 0), lowComplement) | ||
| return (~high + (overflow ? 1 : 0 as DoubleWidth), lowComplement) |
There was a problem hiding this comment.
(Silences a compiler warning about deprecated Strideable operators.)
xwu
commented
Jan 28, 2018
| where Base.Words : Collection, Base.Magnitude.Words : Collection { | ||
| public struct DoubleWidth<Base : FixedWidthInteger> : | ||
| _ExpressibleByBuiltinIntegerLiteral | ||
| where Base.Magnitude : UnsignedInteger, |
There was a problem hiding this comment.
SignedInteger doesn't (and shouldn't, in fact) have the constraint Magnitude : UnsignedInteger.
But for DoubleWidth, we've assumed this constraint in many places (reasonable for SignedInteger & FixedWidthInteger), so we'd better state it. It's necessary now because division helper methods only need to be available for Magnitude, so with this constraint they'll only need to be defined in an extension for unsigned types.
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke benchmark |
1 similar comment
|
@swift-ci Please smoke benchmark |
|
@swift-ci Please smoke benchmark |
|
|
|
@swift-ci Please smoke benchmark |
|
Per #14220, apparently the benchmarks are currently all sticking at CSV parsing. Something to do with the recent String guts changes? |
moiseev
approved these changes
Jan 29, 2018
| } | ||
| let quotient_ = (self.high < (0 as High)) != (other.high < (0 as High)) |
There was a problem hiding this comment.
I think extracting at least this line into a isNegative variable would help readability, even leaving aside the nested ?:s.
| guard DoubleWidth.isSigned else { | ||
| return (DoubleWidth(quotient), DoubleWidth(remainder)) | ||
| } | ||
| let quotient_ = (self.high < (0 as High)) != (other.high.high < (0 as High)) |
There was a problem hiding this comment.
This part looks sufficiently similar to the one above (inside quotientAndRemainder. I wonder if it can be extracted into a separate @_inline(always) function without affecting performance.
There was a problem hiding this comment.
Unfortunately, not possible because they’re dealing with different types (this is a heterogeneous operation).
| @@ -531,7 +536,12 @@ extension DoubleWidth : FixedWidthInteger { | |||
|
|
|||
| @_inlineable // FIXME(sil-serialize-all) | |||
| public static func &>>=(lhs: inout DoubleWidth, rhs: DoubleWidth) { | |||
| let rhs = rhs & DoubleWidth(DoubleWidth.bitWidth &- 1) | |||
| // FIXME(integers): test types with bit widths that aren't powers of 2 | |||
| let rhs = DoubleWidth.bitWidth.nonzeroBitCount == 1 | |||
There was a problem hiding this comment.
This masking piece should also be extracted into a function, I think, which can have an additional benefit of a descriptive name.
| if _slowPath(rhs == (0 as ${Self})) { | ||
| _preconditionFailure("Remainder of division by zero") | ||
| _preconditionFailure("Division by zero") |
There was a problem hiding this comment.
I would argue that the old message was a bit more helpful.
There was a problem hiding this comment.
This was to align with what you get when you write ‘42 % 0’ or ‘42 / 0’—I think it’s important to be clear that the remainder never overflows; but let me see if I can improve on it a little bit.
| ) { | ||
| _preconditionFailure("Overflow in remainder of division") | ||
| if _slowPath(${'lhs == %s.min && rhs == (-1 as %s)' % (Self, Self)}) { | ||
| _preconditionFailure("Division results in an overflow") |
There was a problem hiding this comment.
Same here. With the new messages it is impossible to tell which one of the operations failed in the expression like a / b % c.
|
@swift-ci Please smoke benchmark |
Build comment file:Optimized (O)Regression (8)
Improvement (15)
No Changes (332)
Unoptimized (Onone)Regression (17)
Improvement (21)
No Changes (317)
Hardware Overview |
|
Nice :) |
|
@swift-ci Please smoke test and merge |
1 similar comment
|
@swift-ci Please smoke test and merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements more efficient
DoubleWidthdivision using full-width division primitives available onBase. It's one chunk of changes from #13784, which CI won't test anymore.Also in this PR:
{U}Int64.dividingFullWidthin terms ofDoubleWidth<{U}Int32>.dividingFullWidth(which is required to avoid circularity).Existing
DoubleWidthtests cover this change.remainderReportingOverflowfor builtin types when dividing by-1.A test is added to cover this change.
DoubleWidthmasking shifts.No types exist to test this change.