[stdlib] Implement SE-0233 AdditiveArithmetic #20422

rxwei · 2018-11-08T03:47:08Z

This PR implements proposal SE-0233: Make Numeric Refine a new AdditiveArithmetic Protocol.

rxwei · 2018-11-08T04:14:51Z

@swift-ci please smoke test

stephentyrone · 2018-11-08T13:35:17Z

@swift-ci please test

stephentyrone · 2018-11-08T13:35:41Z

@swift-ci please test source compatibility

swift-ci · 2018-11-08T13:35:52Z

Build failed
Swift Test OS X Platform
Git Sha - 44c2f77

swift-ci · 2018-11-08T13:37:09Z

Build failed
Swift Test Linux Platform
Git Sha - 44c2f77

stephentyrone · 2018-11-08T13:38:39Z

stdlib/public/core/Integers.swift

+//===----------------------------------------------------------------------===//
+
+public protocol AdditiveArithmetic : Equatable {
+  /// The zero value.


Can we expand this comment with a note that zero is the name of the additive identity? Something like
/// zero is the identity element for addition; for any value x,x + .zero == x and .zero + x == x.

Done. There's still no doc comment on the protocol itself, but writing good doc comments would take time :) I added TODO's for that.

stephentyrone · 2018-11-08T13:45:25Z

LGTM aside from minor documentation request. If tests pass, I'm fine with merging as is and adjusting documentation in follow-on patch.

stephentyrone · 2018-11-08T13:46:17Z

@swift-ci please test performance

rxwei · 2018-11-08T18:22:18Z

@swift-ci please smoke test

rxwei · 2018-11-08T18:22:24Z

@swift-ci please test performance

rxwei · 2018-11-08T18:22:36Z

@swift-ci please test source compatibility

test/api-digester/Outputs/stability-stdlib-abi.swift.expected

rxwei · 2018-11-08T18:47:51Z

@swift-ci please test

swift-ci · 2018-11-08T19:16:07Z

Build failed
Swift Test OS X Platform
Git Sha - bc8b623

rxwei · 2018-11-08T19:39:46Z

@swift-ci please test os x platform

swift-ci · 2018-11-08T21:11:08Z

Build failed
Swift Test Linux Platform
Git Sha - b788e55

rxwei · 2018-11-08T21:13:39Z

Not sure why CI says "Build timed out (after 60 minutes)". @shahmishal

xwu · 2018-11-08T22:32:49Z

stdlib/public/core/Integers.swift

+  /// example:
+  ///
+  ///     1 + 2                   // 3
+  ///     -10 + 15                // 5


Examples involving the unary negative operator should probably be removed from the documentation for this protocol.

Sure, but it was originally on Numeric, and Numeric did not have - either. I'd like for this PR to focus on the implementation (to unblock @stephentyrone's SIMD PR, for example), and write a separate patch to refine documentation.

Right. @xwu, can you open a SR bug for this (since we would want to consider it on Numeric anyway) and CC me and @natecook1000?

shahmishal · 2018-11-08T23:25:10Z

Not sure why CI says "Build timed out (after 60 minutes)". @shahmishal

We are looking into why LLDB test suite.

rxwei · 2018-11-09T01:09:41Z

@swift-ci please test

swift-ci · 2018-11-09T03:03:17Z

Build failed
Swift Test OS X Platform
Git Sha - b788e55

swift-ci · 2018-11-09T04:32:26Z

Build failed
Swift Test Linux Platform
Git Sha - b788e55

airspeedswift · 2018-11-09T17:52:18Z

@swift-ci please benchmark

airspeedswift · 2018-11-09T17:52:29Z

@swift-ci please test compiler performance

swift-ci · 2018-11-09T17:57:16Z

Build comment file:

airspeedswift · 2018-11-09T17:59:04Z

oh, duh, can't run the benchmarks until the conflicts are resolved, sorry

rxwei · 2018-11-09T18:18:12Z

@swift-ci please benchmark

airspeedswift · 2018-11-09T18:44:10Z

@swift-ci please benchmark

airspeedswift · 2018-11-09T18:44:32Z

@swift-ci please test compiler performance

swift-ci · 2018-11-09T18:44:54Z

Build comment file:

airspeedswift · 2018-11-09T18:45:30Z

@swift-ci please test

swift-ci · 2018-11-09T18:47:22Z

Build failed
Swift Test Linux Platform
Git Sha - da50193

swift-ci · 2018-11-09T18:48:56Z

Build failed
Swift Test OS X Platform
Git Sha - da50193

swift-ci · 2018-11-09T19:32:07Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
StringEqualPointerComparison	657	571	-13.1%	1.15x
IterateData	1609	1399	-13.1%	1.15x

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
BinaryFloatingPointConversionFromBinaryInteger.o	11305	11621	+2.8%	0.97x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
IterateData	1541	1365	-11.4%	1.13x
StringEqualPointerComparison	628	571	-9.1%	1.10x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
BinaryFloatingPointConversionFromBinaryInteger.o	11209	11525	+2.8%	0.97x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
ArrayOfPOD	858	777	-9.4%	1.10x

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

--------------

rxwei · 2018-11-09T22:28:48Z

@swift-ci please test

swift-ci · 2018-11-09T22:31:00Z

Build failed
Swift Test Linux Platform
Git Sha - 4e2147e

swift-ci · 2018-11-09T22:31:35Z

Build failed
Swift Test OS X Platform
Git Sha - 4e2147e

rxwei · 2018-11-09T22:56:13Z

@swift-ci please smoke test

rxwei · 2018-11-09T22:56:41Z

@swift-ci please smoke test os x

rxwei · 2018-11-10T04:43:38Z

@swift-ci please smoke test and merge

jrose-apple · 2018-11-13T01:57:47Z

This broke the source compat suite:

/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/project_cache/Tagged/Sources/Tagged/Tagged.swift:110:1: error: conditional conformance of type 'Tagged<Tag, RawValue>' to protocol 'Numeric' does not imply conformance to inherited protocol 'AdditiveArithmetic'
extension Tagged: Numeric where RawValue: Numeric {
^

…hm. That's a valid error.

slavapestov · 2018-11-13T02:13:05Z

Can we at least update the proposal to explain the migration path?

The proposed change is fully source-compatible.

This is clearly not true, because conditional conformances don't imply conformance to refined protocols.

It's not even clear to me how one would go about implementing this conformance in a way that works with both 4.2 and 5.0.

rxwei · 2018-11-13T02:33:00Z

This is clearly not true, because conditional conformances don't imply conformance to refined protocols.

Thanks for the tip. It was not clear to me. Given that it passed the core team review, it wasn't clear to other folks either. Could you please suggest a way to fix this? If there's no possible fix, I'll be happy to explain the migration path and update the compatibility suite.

Implement SE-0233 AdditiveArithmetic

44c2f77

rxwei requested review from stephentyrone and airspeedswift November 8, 2018 03:47

Fix test

bc8b623

stephentyrone approved these changes Nov 8, 2018

View reviewed changes

rxwei added 2 commits November 8, 2018 10:18

Update comments

57facc8

Merge branch 'master' of github.com:apple/swift into additive-arithmetic

394be18

airspeedswift requested changes Nov 8, 2018

View reviewed changes

test/api-digester/Outputs/stability-stdlib-abi.swift.expected Outdated Show resolved Hide resolved

Update ABI test

b788e55

rxwei force-pushed the additive-arithmetic branch from 4bdd2fe to b788e55 Compare November 8, 2018 18:42

xwu reviewed Nov 8, 2018

View reviewed changes

Update ABI test

da50193

Merge branch 'master' into additive-arithmetic

4e2147e

benrimmington mentioned this pull request Nov 9, 2018

[stdib] Remove RandomNumberGenerator._fill(bytes:) #20463

Merged

airspeedswift approved these changes Nov 9, 2018

View reviewed changes

Merge branch 'master' into additive-arithmetic

c026851

Merge branch 'master' into additive-arithmetic

aeadc14

swift-ci merged commit 94dc4a6 into swiftlang:master Nov 10, 2018

rxwei deleted the additive-arithmetic branch November 10, 2018 14:10

jrose-apple mentioned this pull request Nov 13, 2018

[ClangImporter] NS_ERROR_ENUMs can be redefined in separate modules #20527

Merged

[stdlib] Implement SE-0233 AdditiveArithmetic #20422

[stdlib] Implement SE-0233 AdditiveArithmetic #20422

Conversation

rxwei commented Nov 8, 2018

rxwei commented Nov 8, 2018

stephentyrone commented Nov 8, 2018

stephentyrone commented Nov 8, 2018

swift-ci commented Nov 8, 2018

swift-ci commented Nov 8, 2018

stephentyrone Nov 8, 2018

Choose a reason for hiding this comment

rxwei Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

stephentyrone commented Nov 8, 2018 • edited Loading

stephentyrone commented Nov 8, 2018

rxwei commented Nov 8, 2018

rxwei commented Nov 8, 2018

rxwei commented Nov 8, 2018

rxwei commented Nov 8, 2018

swift-ci commented Nov 8, 2018

rxwei commented Nov 8, 2018

swift-ci commented Nov 8, 2018

rxwei commented Nov 8, 2018

xwu Nov 8, 2018

Choose a reason for hiding this comment

rxwei Nov 8, 2018

Choose a reason for hiding this comment

stephentyrone Nov 9, 2018

Choose a reason for hiding this comment

xwu Nov 9, 2018

Choose a reason for hiding this comment

shahmishal commented Nov 8, 2018

rxwei commented Nov 9, 2018

swift-ci commented Nov 9, 2018

swift-ci commented Nov 9, 2018

airspeedswift commented Nov 9, 2018

airspeedswift commented Nov 9, 2018

swift-ci commented Nov 9, 2018

Build comment file:

airspeedswift commented Nov 9, 2018

rxwei commented Nov 9, 2018

airspeedswift commented Nov 9, 2018

airspeedswift commented Nov 9, 2018

swift-ci commented Nov 9, 2018

Build comment file:

airspeedswift commented Nov 9, 2018

swift-ci commented Nov 9, 2018

swift-ci commented Nov 9, 2018

swift-ci commented Nov 9, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

rxwei commented Nov 9, 2018

swift-ci commented Nov 9, 2018

swift-ci commented Nov 9, 2018

rxwei commented Nov 9, 2018

rxwei commented Nov 9, 2018

rxwei commented Nov 10, 2018

jrose-apple commented Nov 13, 2018 • edited Loading

slavapestov commented Nov 13, 2018 • edited Loading

rxwei commented Nov 13, 2018 • edited Loading

rxwei Nov 8, 2018 •

edited

Loading

stephentyrone commented Nov 8, 2018 •

edited

Loading

jrose-apple commented Nov 13, 2018 •

edited

Loading

slavapestov commented Nov 13, 2018 •

edited

Loading

rxwei commented Nov 13, 2018 •

edited

Loading