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

[stdlib] DoubleWidth Implementation #9367

Merged
merged 22 commits into from Jun 5, 2017

Conversation

Projects
None yet
5 participants
@natecook1000
Member

natecook1000 commented May 6, 2017

stdlib/public/core/Integers.swift.gyb Outdated
public func quotientAndRemainder(dividingBy other: DoubleWidth)
-> (quotient: DoubleWidth, remainder: DoubleWidth) {
let isNegative = (self < DoubleWidth()) != (other < DoubleWidth())

This comment has been minimized.

@dabrahams

dabrahams May 6, 2017

Member

We can't use 0 here?

This comment has been minimized.

@natecook1000

natecook1000 May 7, 2017

Member

I think this was from before I implemented the integer literal init—works now for values that aren't larger than an Int.

stdlib/public/core/Integers.swift.gyb Outdated
}
public var littleEndian: DoubleWidth<T> {
fatalError()
public var littleEndian: DoubleWidth {

This comment has been minimized.

@dabrahams

dabrahams May 6, 2017

Member

Is the code for the methods that use byteSwapped all boilerplate that gets repeated for all integer types, and if so can we factor it out (e.g. into a default implementation)?

This comment has been minimized.

@natecook1000

natecook1000 May 7, 2017

Member

It is! Factored that out into a FixedWidthInteger default implementation—byteSwapped is the customization point. I didn't remove the implementations from the standard integer types.

This comment has been minimized.

@natecook1000

natecook1000 May 8, 2017

Member

Why not?

Compelling argument! 😂 Those are out now too; tests updated.

This comment has been minimized.

@xwu

xwu May 8, 2017

Collaborator

Are there not performance implications to losing the concrete implementations?

This comment has been minimized.

@natecook1000

natecook1000 May 8, 2017

Member

I would think specialization would take care of that, but I could be wrong…

This comment has been minimized.

@dabrahams

dabrahams May 9, 2017

Member

You could write a benchmark, but looking at generated code is probably enough.

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 7, 2017

@natecook1000 natecook1000 force-pushed the natecook1000:nc-doublewidth branch May 12, 2017

@natecook1000 natecook1000 force-pushed the natecook1000:nc-doublewidth branch to 5ab34bf May 25, 2017

@natecook1000

This comment has been minimized.

Member

natecook1000 commented May 25, 2017

@swift-ci Please smoke test

@natecook1000 natecook1000 changed the title from [stdlib] Partial implementation of DoubleWidth to [stdlib] DoubleWidth Implementation May 25, 2017

@natecook1000

This comment has been minimized.

Member

natecook1000 commented May 25, 2017

@swift-ci Please smoke test

@moiseev

Just a thought... Now that the DoubleWidth<T> has implementation, it is possible and quite interesting to create a 2- or 3-bit FixedWidthInteger and test DoubleWidth<T> exhaustively.

do {
let x = UInt1024.max
let (y, o) = x.addingReportingOverflow(1)
expectEqual(y, 0)

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

FYI: StdlibUnittest defines expectEquals for tuples of up to 4 equatable elements.

where Magnitude : BinaryInteger, Magnitude.Magnitude == Magnitude
{
// FIXME(ABI) (Recursive Protocol Constraints): should add:
// where Magnitude : BinaryInteger

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Isn't it added 3 lines above?

This comment has been minimized.

@natecook1000
#endif
% end
}
/// A representation of this integer with the byte order swapped.
public var byteSwapped: ${Self} {

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Since it's the basis of all the other endianness conversion operations, probably worth making it @_transparent or at least @inline(__always).

}
// integer
//
public var magnitude: DoubleWidth<Low> {
if T.isSigned && _storage.high < 0 {
return (DoubleWidth<T>() - self).magnitude
if Base.isSigned && _storage.high < 0 {

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

_storage.high < (0 as High) maybe?

}

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Unnecessary indentation.

}
public func remainderReportingOverflow(dividingBy other: DoubleWidth)
-> (partialValue: DoubleWidth, overflow: ArithmeticOverflow) {
if other == 0 {

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Shouldn't remainder overflow in case of .min / -1 as well?

This comment has been minimized.

@natecook1000

natecook1000 May 25, 2017

Member

This is funny, because the remainder (0) is representable, but the quotient is not.

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

I now remember the discussion about integers whether the remainder should overflow in this case, and I believe we ended up agreeing that it should not.
Regardless, the behavior should be consistent with that of built-in integer types.

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Huh! It is a compiler error =) So I guess we stick to the plan. Overflow that is.

(swift) Int8.min.remainderReportingOverflow(dividingBy: -1 as Int8)
<REPL Input>:1:10: error: division '-128 % -1' results in an overflow
Int8.min.remainderReportingOverflow(dividingBy: -1 as Int8)
         ^
}
% end
public static func <<=(lhs: inout DoubleWidth, rhs: DoubleWidth) {
if rhs < DoubleWidth((0, 0)) {

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

DoubleWidth should also conform to ExpressibleByIntegerLiteral right? So this can be replaced by just rhs < (0 as DoubleWidth)? Not a big win in readability, but at least no ((...)) ;-)

}
% end
public static func <<=(lhs: inout DoubleWidth, rhs: DoubleWidth) {
if rhs < DoubleWidth((0, 0)) {
lhs >>= DoubleWidth((0, 0)) - rhs

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Prefix - should be able to take care of this as well... I think..

This comment has been minimized.

@natecook1000

natecook1000 May 25, 2017

Member

Prefix - is only available for signed DoubleWidths, so I'm subtracting from zero in non-constrained methods.

// Shift is exactly the width of `Base`, so low -> high.
if Int(rhs._storage.low) == Base.bitWidth {
lhs = DoubleWidth((High(extendingOrTruncating: lhs._storage.low), numericCast(0)))

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

numericCast should not be necessary. Without it the type context is unambiguous, and literal would get the right type... Well, in ideal world, I mean.

}
// other
//
public init(_builtinIntegerLiteral x: _MaxBuiltinIntegerType) {
fatalError("Method must be overridden")
// FIXME: This won't work if `x` is out of range for `Int`
self.init(Int(_builtinIntegerLiteral: x))

This comment has been minimized.

@moiseev

moiseev May 25, 2017

Member

Will it change anything if we use Int64 explicitly? I mean, it's not perfect still, but at least on 32-bit platforms it would support larger literals.

@moiseev

This comment has been minimized.

Member

moiseev commented May 25, 2017

Overall @natecook1000 this is AMAZING! Thanks a ton! 👍

natecook1000 added some commits May 25, 2017

Incorporate feedback from @moiseev
- Also clean up some 80-column issues
- And improve some tests from before literal expressibility
let result = DoubleWidth(extendingOrTruncating: product)
let isNegative = (self < (0 as DoubleWidth)) != (rhs < (0 as DoubleWidth))
let didCarry = isNegative ? carry != ~0 : carry != 0

This comment has been minimized.

@xwu

xwu May 26, 2017

Collaborator

Yikes, this is where the heterogeneous operators really start biting. Presumably, you didn't mean ~0 as in Int(-1).

let didCarry = isNegative ? carry != ~(0 as DoubleWidth) : carry != (0 as DoubleWidth)

This comment has been minimized.

@natecook1000

natecook1000 May 26, 2017

Member

Yikes, thanks for finding this!

@moiseev

This comment has been minimized.

Member

moiseev commented May 26, 2017

@xwu FWIW, we did not modify our benchmark code at all when the new integers landed. So if there was any degradation in performance it was there and was deemed acceptable. I remember playing with explicit type context in various places, and measuring difference in compilation time as well as performance of the resulting code, and not seeing much difference.

The last point you made, though, is really frightening. I'm talking about x != ~0. Testing it now.

@moiseev

This comment has been minimized.

Member

moiseev commented May 26, 2017

func f<T : BinaryInteger>(_ x: T) -> Bool {
  return x != ~0
}

func g<T : BinaryInteger>(_ x: T) -> Bool {
  return x != (~0 as T)
}

print(f(UInt.max))
print(g(UInt.max))

Outputs:

true
false

And this is bad. 😞

@moiseev

This comment has been minimized.

Member

moiseev commented May 26, 2017

@inline(never)
func f<T : BinaryInteger>(_ x: T) -> Bool {
  return x != 0
}

@inline(never)
func g<T : BinaryInteger>(_ x: T) -> Bool {
  return x != (0 as T)
}

public var result = false

for i in 0 ..< 10_000_000_000 {
  result = result || f(i)
  // result = result || g(i)
}

Rough performance measurements of this code for f and g are virtually the same (8.062 for f vs 8.014 for g).

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

So, this is not simply a problem with heterogeneous comparison, but an interaction with the way literals and overloading work.

@DougGregor We may need to have that discussion about literals sooner than we thought.

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

@natecook1000 Regarding #9367 (comment) (GH doesn't give me a place to reply), I can't see enough context to guess what assumption you're talking about.

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

@natecook1000

This comment has been minimized.

Member

natecook1000 commented May 26, 2017

The assumption is that a fixed width integer's Magnitude will always have the same bitWidthas the type itself. The assumption is explicit with DoubleWidth, since it uses Magnitude as the lower half.

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

@xwu

This comment has been minimized.

Collaborator

xwu commented May 26, 2017

@dabrahams Well, it was a presumption based on reading the code. Maxim has pointed out that the difference is negligible when the type is Int, and it appears that's the case even with actual heterogeneous comparison, so I guess that's not a concern:

    @inline(never)
    func f<T : BinaryInteger>(_ x: T) -> Bool {
      return x != 0
    }
    var result = false
    measure {
      for i in 0..<1_000_000_000 {
        result = result || f(UInt16(i % Int(UInt16.max)))
      }
    } // 6.962 seconds
    @inline(never)
    func g<T : BinaryInteger>(_ x: T) -> Bool {
      return x != (0 as T)
    }
    var result = false
    measure {
      for i in 0..<1_000_000_000 {
        result = result || g(UInt16(i % Int(UInt16.max)))
      }
    } // 7.001 seconds
@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

Fortunately looking at the code isn't a good way to tell 😉
If you inspect the SIL (or at least the generated asssmbly) they should be identical.

@dabrahams

This comment has been minimized.

Member

dabrahams commented May 26, 2017

On second thought, I'm wrong about the matching bitWidth requirement on Magnitude. A big point of the FixedWidthInteger protocol is enabling the construction of multiple precision integers, and DoubleWidth is supposed to be a test case for that. Obviously this means we need to add the requirement to FixedWidthInteger!

@natecook1000

This comment has been minimized.

Member

natecook1000 commented May 26, 2017

@swift-ci Please smoke test

@moiseev

This comment has been minimized.

Member

moiseev commented Jun 2, 2017

@natecook1000 are you happy with this one or would like to change some more?

@natecook1000

This comment has been minimized.

Member

natecook1000 commented Jun 2, 2017

@moiseev This is ready to go! 👍

@moiseev

This comment has been minimized.

Member

moiseev commented Jun 2, 2017

@swift-ci Please Test Source Compatibility

@moiseev

This comment has been minimized.

Member

moiseev commented Jun 2, 2017

@swift-ci Please test

@@ -2361,8 +2395,7 @@ ${operatorComment('&' + x.operator, True)}
//===----------------------------------------------------------------------===//
/// An integer type that can represent only nonnegative values.
public protocol UnsignedInteger : BinaryInteger
where Magnitude : BinaryInteger { }
public protocol UnsignedInteger : BinaryInteger { }

This comment has been minimized.

@xwu

xwu Jun 2, 2017

Collaborator

@natecook1000 I didn't notice this change the first time round. Is there a reason that UnsignedInteger (and, below, SignedInteger) are losing the constraint where Magnitude : BinaryInteger?

This comment has been minimized.

@moiseev

moiseev Jun 2, 2017

Member

I think it's now in BinaryInteger itself.

This comment has been minimized.

@natecook1000

natecook1000 Jun 3, 2017

Member

Yep! Constraints on associated types can be recursive as long as the associated type is declared in a higher protocol.

Hashable, Numeric, CustomStringConvertible, Strideable {
Hashable, Numeric, CustomStringConvertible, Strideable
where Magnitude : BinaryInteger, Magnitude.Magnitude == Magnitude

This comment has been minimized.

@moiseev

moiseev Jun 2, 2017

Member

@xwu here

@moiseev

This comment has been minimized.

Member

moiseev commented Jun 3, 2017

@swift-ci Please Test Source Compatibility

@moiseev moiseev merged commit 0c10834 into apple:master Jun 5, 2017

5 checks passed

Swift Source Compatibility Suite on macOS Platform Build finished.
Details
Swift Test Linux Platform 9907 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 49790 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

natecook1000 added a commit to natecook1000/swift that referenced this pull request Jun 7, 2017

@natecook1000 natecook1000 deleted the natecook1000:nc-doublewidth branch Jun 8, 2017

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