[stdlib] DoubleWidth Implementation #9367

natecook1000 · 2017-05-06T07:47:34Z

dabrahams · 2017-05-06T16:45:11Z

stdlib/public/core/Integers.swift.gyb

+
+  public func quotientAndRemainder(dividingBy other: DoubleWidth)
+    -> (quotient: DoubleWidth, remainder: DoubleWidth) {
+    let isNegative = (self < DoubleWidth()) != (other < DoubleWidth())


We can't use 0 here?

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

dabrahams · 2017-05-06T16:47:32Z

stdlib/public/core/Integers.swift.gyb

  }

-  public var littleEndian: DoubleWidth<T> {
-    fatalError()
+  public var littleEndian: DoubleWidth {


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)?

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.

Why not?

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

Are there not performance implications to losing the concrete implementations?

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

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

dabrahams · 2017-05-07T17:59:14Z

on Sun May 07 2017, Nate Cook <notifications-AT-i.8713187.xyz> wrote:

I didn't remove the implementations from the standard integer types.

Why not?

natecook1000 · 2017-05-25T03:26:49Z

@swift-ci Please smoke test

natecook1000 · 2017-05-25T08:49:42Z

@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.

moiseev · 2017-05-25T18:04:09Z

test/stdlib/Integers.swift.gyb

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


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

moiseev · 2017-05-25T18:06:47Z

stdlib/public/core/Integers.swift.gyb

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


Isn't it added 3 lines above?

moiseev · 2017-05-25T18:11:11Z

stdlib/public/core/Integers.swift.gyb

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


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

moiseev · 2017-05-25T18:15:01Z

stdlib/public/core/Integers.swift.gyb

  }

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


_storage.high < (0 as High) maybe?

moiseev · 2017-05-25T18:16:54Z

stdlib/public/core/Integers.swift.gyb

  }
-
+    


Unnecessary indentation.

moiseev · 2017-05-25T19:10:39Z

stdlib/public/core/Integers.swift.gyb

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


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

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

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.

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) ^

moiseev · 2017-05-25T19:14:20Z

stdlib/public/core/Integers.swift.gyb

  }
 % end

+  public static func <<=(lhs: inout DoubleWidth, rhs: DoubleWidth) {
+    if rhs < DoubleWidth((0, 0)) {


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 ((...)) ;-)

moiseev · 2017-05-25T19:14:47Z

stdlib/public/core/Integers.swift.gyb

  }
 % end

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


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

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

moiseev · 2017-05-25T19:16:27Z

stdlib/public/core/Integers.swift.gyb

+
+    // 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)))


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.

moiseev · 2017-05-25T19:19:51Z

stdlib/public/core/Integers.swift.gyb

  }

  // 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))


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 · 2017-05-25T19:25:59Z

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

- Also clean up some 80-column issues - And improve some tests from before literal expressibility

moiseev · 2017-05-26T01:20:42Z

@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 · 2017-05-26T01:34:34Z

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 · 2017-05-26T01:50:12Z

@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 · 2017-05-26T02:24:28Z

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 · 2017-05-26T02:39:21Z

@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 · 2017-05-26T02:43:34Z

on Thu May 25 2017, Nate Cook <notifications-AT-i.8713187.xyz> wrote:

A three- or five-bit integer would be particularly good for testing, since there could easily be power-of-two assumptions in the code. And like, does a type's `Magnitude` need to have the same bit width as the type?

OK, I see now what you were referring to. No, a type's Magnitude doesn't need to have the same bit width, but as Max wrote, it's a good idea to use a Magnitude of minimal width.

…

-- -Dave

natecook1000 · 2017-05-26T02:43:35Z

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 · 2017-05-26T03:17:52Z

on Thu May 25 2017, Nate Cook <notifications-AT-i.8713187.xyz> wrote:

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

That relationship needn't be forced, though I suppose it would complicate overflow detection if that were not the case. In principle, the compiler should be able to optimize that complication away when the `bitWidth`'s do match, but that's a lot of code complexity for no gain in practice. I think it would be fine to make it a requirement on `DoubleWidth's` parameter without pushing it into the protocol.

…

-- -Dave

dabrahams · 2017-05-26T04:48:28Z

the heterogeneous comparison with `0 as IntegerLiteralType` is less performant,

Oh, that is sad. Hopefully that will get fixed before we ship!

…

-- -Dave

xwu · 2017-05-26T08:13:37Z

@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 · 2017-05-26T13:25:12Z

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 · 2017-05-26T13:31:13Z

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 · 2017-05-26T20:59:54Z

@swift-ci Please smoke test

moiseev · 2017-06-02T01:22:24Z

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

natecook1000 · 2017-06-02T15:27:55Z

@moiseev This is ready to go! 👍

moiseev · 2017-06-02T17:51:22Z

@swift-ci Please Test Source Compatibility

moiseev · 2017-06-02T18:39:59Z

@swift-ci Please test

xwu · 2017-06-02T21:14:39Z

stdlib/public/core/Integers.swift.gyb

@@ -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 { }


@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?

I think it's now in BinaryInteger itself.

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

moiseev · 2017-06-02T21:18:21Z

stdlib/public/core/Integers.swift.gyb

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


moiseev · 2017-06-03T02:07:16Z

@swift-ci Please Test Source Compatibility

[stdlib] DoubleWidth Implementation

dabrahams reviewed May 6, 2017

View reviewed changes

natecook1000 force-pushed the nc-doublewidth branch from 23033e8 to 0dbb5f4 Compare May 12, 2017 21:12

natecook1000 added 16 commits May 24, 2017 19:16

[stdlib] Partial implementation of DoubleWidth

8926df9

Use literals instead of DoubleWidth inits

b2676f9

Convert endian symbols to default implementations

c866bcf

Add negation operator/method to DoubleWidth

a539919

A few tests for DoubleWidth

c8f4c4b

Provide implementations or masking bitshift operators

b72449b

Remove endian operations from fixed-width ints

b935be8

Add recursive constraints for integer Magnitude

788f4e8

Update tests for endian default implementations

b6af57a

Fix overflow issues

2edbb08

Slightly better DoubleWidth testing

4a24a96

Implement tricky full-width multiply/divides

1e59f65

Fix overflow issue

7353042

Shortcut on binary division

06490e6

More tests for DoubleWidth

40f6869

Add floating-point initializers and tests

5ab34bf

natecook1000 force-pushed the nc-doublewidth branch from 0dbb5f4 to 5ab34bf Compare May 25, 2017 02:51

Revert multipliedFullWidth for 64-bit ints on 32-bit platforms

e5fb871

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

moiseev reviewed May 25, 2017

View reviewed changes

natecook1000 added 3 commits May 25, 2017 15:04

Catch overflow into high bit in signed multiplication

237d877

Incorporate feedback from @moiseev

1149eea

- Also clean up some 80-column issues - And improve some tests from before literal expressibility

Add an abstract for DoubleWidth

2f9bf3c

Additional notes from @xwu

2612e00

One more from @xwu + 80-column fixes

42efcdb

xwu reviewed Jun 2, 2017

View reviewed changes

moiseev reviewed Jun 2, 2017

View reviewed changes

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

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

Merge pull request swiftlang#9367 from natecook1000/nc-doublewidth

1c946a4

[stdlib] DoubleWidth Implementation

natecook1000 mentioned this pull request Jun 7, 2017

[4.0] Add DoubleWidth implementation #10157

Closed

natecook1000 deleted the nc-doublewidth branch June 8, 2017 19:50

moiseev mentioned this pull request Jul 24, 2017

[swift-4.0-branch][stdlib] Remove DoubleWidth<T> #11141

Merged

xwu mentioned this pull request Mar 5, 2018

Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

Closed

xwu mentioned this pull request Nov 3, 2020

[Constraint System] Implement heuristics for linked operator expressions in the solver proper. #34399

Merged

xwu mentioned this pull request Nov 7, 2020

[SR-13755] Convenience operators on BinaryInteger causing subtle bugs due to unintuitive type inference #56152

Open

[stdlib] DoubleWidth Implementation #9367

[stdlib] DoubleWidth Implementation #9367

Uh oh!

Conversation

natecook1000 commented May 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams commented May 7, 2017 via email

Uh oh!

natecook1000 commented May 25, 2017

Uh oh!

natecook1000 commented May 25, 2017

Uh oh!

moiseev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moiseev commented May 25, 2017

Uh oh!

moiseev commented May 26, 2017

Uh oh!

moiseev commented May 26, 2017

Uh oh!

moiseev commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dabrahams commented May 26, 2017

Uh oh!

dabrahams commented May 26, 2017

Uh oh!

dabrahams commented May 26, 2017 via email

Uh oh!

natecook1000 commented May 26, 2017

Uh oh!

dabrahams commented May 26, 2017 via email

Uh oh!

moiseev commented May 26, 2017 •

edited

Loading