BinaryInteger distance(to:) fixes #71387
Closed
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
…:) method. - `UInt.max.distance(to: UInt.max / 2)`, etc. - `Int64(Int.max).distance(to: Int64(-1 as Int))`, etc.
|
I'm evaluating this by looking at specialized There are two cases to consider: (A) the case where In the case (A), this fix produces identical output as before for an unsigned type, and the output is as good as for In the narrowing case (B):
Note that the "Distance is not representable in Int" message only matters in the narrowing case, and (at the moment) only shows up in debug mode. It does seem valuable to have it though. Godbolt page: https://swift.godbolt.org/z/4jsoYzGTe |
|
@swift-ci please test |
Well, there are three checked operations: the subtraction in (A), the initializer in (B), and the subtraction in (B). (A) SubtractionThis trap is easy to defer, albeit doing so is a bit more verbose. Note This does not affect the assembly. let result = Int(truncatingIfNeeded: other).subtractingReportingOverflow(Int(truncatingIfNeeded: self))
if !result.overflow {
return result.partialValue
}(B) InitializerThis trap is similarly easy to defer: Note This removes a trap from the assembly. The assembly of using if let result = Int(exactly: other - self) {
return result
}(B) SubtractionDeferring this trap devolves into the unfixed version with extra comparisons, absent recoverable arithmetic. Warning The assembly would be worse than output.signed1(n: Swift.Int64, a: Swift.Int64) -> Swift.Int32. Godbolt page: https://swift.godbolt.org/z/q7qdGazc7 |
This appears to be a result of doing the semantically correct thing: comparing Stride.min.magnitude. The chef's choiceSo, uhm. If you don't care about the error message, I've got some great unsigned assembly cooking: Godbolt page: https://swift.godbolt.org/z/5YKdjY6x6 Note I'm not sure where the assembly trap is hiding, but // this is the (B) thing-y, not the proposed generic method...
public func distance4(to other: Self) -> Int32 {
if Self.isSigned {
if self.bitWidth <= Int32.bitWidth && other.bitWidth <= Int32.bitWidth {
return Int32(truncatingIfNeeded: other) - Int32(truncatingIfNeeded: self)
} else {
return Int32(exactly: other - self)!
}
} else {
if self > other {
let result: Self = self - other
if result.bitWidth < Int32.bitWidth || Self(truncatingIfNeeded: Int32.min.magnitude) - result >= (0 as Self) {
return ~Int32(truncatingIfNeeded: result) &+ 1
}
} else {
let result: Self = other - self
if result.bitWidth < Int32.bitWidth || Self(truncatingIfNeeded: Int32.max.magnitude) - result >= (0 as Self) {
return Int32(truncatingIfNeeded: result)
}
}
}
preconditionFailure("this is unreachable because the comparison either traps or succeeds")
} |
|
I've played with this some more and the subtraction trap trick still yields the best result. Godbolt: https://swift.godbolt.org/z/r6xKqhz71 BeforeAssemblySourceextension BinaryInteger {
public func distanceV1<T>(to other: Self) -> T where T: SignedInteger & FixedWidthInteger {
if !Self.isSigned {
if self > other {
if let result = T(exactly: self - other) {
return -result
}
} else {
if let result = T(exactly: other - self) {
return result
}
}
} else {
let isNegative = self < (0 as Self)
if isNegative == (other < (0 as Self)) {
if let result = T(exactly: other - self) {
return result
}
} else {
if let result = T(exactly: self.magnitude + other.magnitude) {
return isNegative ? result : -result
}
}
}
preconditionFailure("Distance is not representable in Int")
}
}AfterAssemblySourceextension BinaryInteger {
public func distanceV2<T>(to other: Self) -> T where T: SignedInteger & FixedWidthInteger {
if Self.isSigned {
if self.bitWidth <= T.bitWidth && other.bitWidth <= T.bitWidth {
return T(truncatingIfNeeded: other) - T(truncatingIfNeeded: self)
} else {
return T(exactly: other - self)!
}
} else {
if self > other {
let result: Self = self - other
if result.bitWidth >= T.bitWidth {
// This subtraction traps when the result does not fit.
_ = Self(truncatingIfNeeded: T.min.magnitude) - result
}
return ~T(truncatingIfNeeded: result) &+ 1
} else {
let result: Self = other - self
if result.bitWidth >= T.bitWidth {
// This subtraction traps when the result does not fit.
_ = Self(truncatingIfNeeded: T.max.magnitude) - result
}
return T(truncatingIfNeeded: result)
}
}
}
} |
|
That's impressive! My preference would be to not lose the message output by |
|
Here's a version where all of the traps have been deferred to the precondition failure at the end. It is the original version with the off-by-one calls to init(exactly:) replaced by inline min/max magnitude comparisons, plus a fast path for smaller or similar signed integers.
Godbolt: https://swift.godbolt.org/z/7fhv84obP Assemblyextension BinaryInteger {
public func distanceWithHeroicEffortsToPreserveTheErrorMessage<T>(to other: Self) -> T where T: SignedInteger & FixedWidthInteger {
if Self.isSigned {
if self.bitWidth <= T.bitWidth && other.bitWidth <= T.bitWidth {
let result = T(truncatingIfNeeded: other).subtractingReportingOverflow(T(truncatingIfNeeded: self))
if !result.overflow {
return result.partialValue
}
} else {
let isNegative = self < (0 as Self)
if isNegative == (other < (0 as Self)) {
if let result = T(exactly: other - self) {
return result
}
} else {
let result = self.magnitude + other.magnitude
if isNegative {
if result <= T.max.magnitude {
return T(truncatingIfNeeded: result)
}
} else {
if result <= T.min.magnitude {
return ~T(truncatingIfNeeded: result) &+ 1
}
}
}
}
} else {
if self <= other {
let result: Self = other - self
if result.bitWidth < T.bitWidth || result <= Self(truncatingIfNeeded: T.max.magnitude) {
return T(truncatingIfNeeded: result)
}
} else {
let result: Self = self - other
if result.bitWidth < T.bitWidth || result <= Self(truncatingIfNeeded: T.min.magnitude) {
return ~T(truncatingIfNeeded: result) &+ 1
}
}
}
preconditionFailure("Distance is not representable in Int")
}
} |
I truly appreciate the efforts! |
swiftlang#71387). This new version is the original distance(to:) method but with the off-by-one calls to init(exactly:) replaced by inline min/max magnitude comparisons, plus a fast path for small and same-size signed integers. Heroic efforts went into deferring the overflow trap until the precondition failure at the end, per code review.
|
I have updated the pull request to match the Heroic ™️ version. Note that the signed narrowing case would be trivial with recoverable arithmetic: let large = other.subtractingReportingOverflow(self)
if !large.overflow, let result = Int(exactly: large.partialValue) {
return result
}The method is not easily testable, so here's a testable format (click to show):import XCTest
final class DistanceToTestsForStdlibIntegers: XCTestCase {
func test() {
check({ (a: Int64, b: Int64) -> Int64? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: Int64, b: Int64) -> Int32? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: Int32, b: Int32) -> Int64? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: Int32, b: Int32) -> Int32? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: UInt64, b: UInt64) -> Int64? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: UInt64, b: UInt64) -> Int32? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: UInt32, b: UInt32) -> Int64? in a.distanceButGenericWithOptionalResult(to: b) })
check({ (a: UInt32, b: UInt32) -> Int32? in a.distanceButGenericWithOptionalResult(to: b) })
}
func check<T, U>(_ distance: (T, T) -> U?, file: StaticString = #file, line: UInt = #line) where T: FixedWidthInteger, U: SignedInteger & FixedWidthInteger {
if T.isSigned, T.bitWidth >= U.bitWidth {
XCTAssertEqual(distance(T(U.max) - 0, T(-1 as U)), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.max) - 0, T(-2 as U)), nil, file: file, line: line)
XCTAssertEqual(distance(T(U.max) - 1, T(-2 as U)), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.max) - 1, T(-3 as U)), nil, file: file, line: line)
XCTAssertEqual(distance(T( 0 as U), T(U.max) - 0), U.max, file: file, line: line)
XCTAssertEqual(distance(T(-2 as U), T(U.max) - 1), nil, file: file, line: line)
XCTAssertEqual(distance(T(-1 as U), T(U.max) - 1), U.max, file: file, line: line)
XCTAssertEqual(distance(T(-1 as U), T(U.max) - 0), nil, file: file, line: line)
}
if !T.isSigned, T.bitWidth >= U.bitWidth {
XCTAssertEqual(distance(T(U.max) + 1, T(0 as U)), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.max) + 2, T(0 as U)), nil, file: file, line: line)
XCTAssertEqual(distance(T(U.max) + 2, T(1 as U)), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.max) + 3, T(1 as U)), nil, file: file, line: line)
XCTAssertEqual(distance(T(0 as U), T(U.max) + 0), U.max, file: file, line: line)
XCTAssertEqual(distance(T(0 as U), T(U.max) + 1), nil, file: file, line: line)
XCTAssertEqual(distance(T(1 as U), T(U.max) + 1), U.max, file: file, line: line)
XCTAssertEqual(distance(T(1 as U), T(U.max) + 2), nil, file: file, line: line)
}
agnostic: if T.bitWidth < U.bitWidth {
XCTAssertEqual(distance(T.max, T.min), U(T.min) - U(T.max), file: file, line: line)
XCTAssertEqual(distance(T.min, T.max), U(T.max) - U(T.min), file: file, line: line)
}
agnostic: if T.bitWidth > U.bitWidth {
XCTAssertEqual(distance(T(U.max) + T(U.max) + 1, T(U.max)), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.max) + T(U.max) + 2, T(U.max)), nil, file: file, line: line)
XCTAssertEqual(distance(T(U.max), T(U.max) + T(U.max) + 0), U.max, file: file, line: line)
XCTAssertEqual(distance(T(U.max), T(U.max) + T(U.max) + 1), nil, file: file, line: line)
guard T.isSigned else { break agnostic }
XCTAssertEqual(distance(T(U.min), T(U.min) + T(U.min) + 0), U.min, file: file, line: line)
XCTAssertEqual(distance(T(U.min), T(U.min) + T(U.min) - 1), nil, file: file, line: line)
XCTAssertEqual(distance(T(U.min) + T(U.min) + 1, T(U.min)), U.max, file: file, line: line)
XCTAssertEqual(distance(T(U.min) + T(U.min) + 0, T(U.min)), nil, file: file, line: line)
}
}
}
extension BinaryInteger {
public func distanceButGenericWithOptionalResult<T>(to other: Self) -> T? where T: SignedInteger & FixedWidthInteger {
if Self.isSigned {
if self.bitWidth <= T.bitWidth && other.bitWidth <= T.bitWidth {
let result = T(truncatingIfNeeded: other).subtractingReportingOverflow(T(truncatingIfNeeded: self))
if !result.overflow {
return result.partialValue
}
} else {
let isNegative = self < (0 as Self)
if isNegative == (other < (0 as Self)) {
if let result = T(exactly: other - self) {
return result
}
} else {
let result: Magnitude = self.magnitude + other.magnitude
if isNegative {
if result <= T.max.magnitude {
return T(truncatingIfNeeded: result)
}
} else {
if result <= T.min.magnitude {
return ~T(truncatingIfNeeded: result) &+ 1
}
}
}
}
} else {
if self <= other {
let result: Self = other - self
if result.bitWidth < T.bitWidth || result <= Self(truncatingIfNeeded: T.max.magnitude) {
return T(truncatingIfNeeded: result)
}
} else {
let result: Self = self - other
if result.bitWidth < T.bitWidth || result <= Self(truncatingIfNeeded: T.min.magnitude) {
return ~T(truncatingIfNeeded: result) &+ 1
}
}
}
return nil // _preconditionFailure("Distance is not representable in \(T.self)")
}
} |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@oscbyspro your PR and the many options it presents are being looked at. |
|
|
For what it's worth, I think trapping the big-signed-integer subtraction is a better option because it's so straightforward compared to the heroic sign-magnitude round trip. |
|
After some discussion with @stephentyrone, we've concluded that we should simply have the best possible implementation in the standard library, and that suboptimal error messages are a compiler issue that should be fixed (see #72411). With that in mind, let's get back to the best possible code-gen option that you found earlier, and we should merge that. Thanks! |
|
Cool. I ran some more tests, and it turns out that the func identity<T>(_ x: T) -> T { x }
// This does not trap in -O mode..?
_ = identity(UInt.min) - identity(UInt.max)Edit: I found this quite surprising, so I opened (#72455) about it. |
…on (swiftlang#71387). This version: 1. drops the large-signed-integer debug message in favor of a faster approach 2. adds a fast path for smaller unsigned integers because the result always fits 3. uses a more performant comparison for smaller and same-size unsigned integers
|
I have picked the most performant version in the latest commit. Important The unsigned part is new, so you might want to take a closer look at it. Here's the inline version of it...extension BinaryInteger {
@inlinable
@inline(__always)
public func distance(to other: Self) -> Int {
if Self.isSigned {
if self.bitWidth <= Int.bitWidth && other.bitWidth <= Int.bitWidth {
let lhs = Int(truncatingIfNeeded: self)
let rhs = Int(truncatingIfNeeded: other)
let result = rhs.subtractingReportingOverflow(lhs)
if !result.overflow {
return result.partialValue
}
} else {
// Use trapping subtraction for performance.
if let result = Int(exactly: other - self) {
return result
}
}
} else {
if self.bitWidth < Int.bitWidth && other.bitWidth < Int.bitWidth {
// Smaller unsigned integers always fit.
let lhs = Int(truncatingIfNeeded: self)
let rhs = Int(truncatingIfNeeded: other)
return rhs &- lhs
} else if self <= other {
let result: Self = other - self
let distance = Int(truncatingIfNeeded: result)
// The zero comparison generates better code in Swift 5.10.
if result.bitWidth <= Int.bitWidth {
if distance >= Int.zero {
return distance
}
} else {
if result <= Self(truncatingIfNeeded: Int.max.magnitude) {
return distance
}
}
} else {
let result: Self = self - other
let distance = ~Int(truncatingIfNeeded: result) &+ 1
// The zero comparison generates better code in Swift 5.10.
if result.bitWidth <= Int.bitWidth {
if distance < Int.zero {
return distance
}
} else {
if result <= Self(truncatingIfNeeded: Int.min.magnitude) {
return distance
}
}
}
}
_preconditionFailure("Distance is not representable in Int")
}
}Saves all but one message
Improved unsigned assemblyI managed to improve the unsigned assembly for sizes
TestsI added some Godbolt: https://swift.godbolt.org/z/x3Pqncbx3Show OLD signed results...Show OLD unsigned results...Show NEW signed results...Show NEW unsigned results... |
|
Here's a much simpler version, with identical specialized Godbolt: https://swift.godbolt.org/z/4Mqjrex6E extension BinaryInteger {
@inlinable
@inline(__always)
public func distance(to other: Self) -> Int {
if self.bitWidth < Int.bitWidth && other.bitWidth < Int.bitWidth {
// Smaller integers always fit.
return Int(truncatingIfNeeded: other) &- Int(truncatingIfNeeded: self)
} else if Self.isSigned || self <= other {
// Use trapping subtraction for performance.
if let result = Int(exactly: other - self) {
return result
}
} else {
// This type is unsigned and the distance is negative here.
let absolute = self - other
let distance = ~Int(truncatingIfNeeded: absolute) &+ 1
// The zero comparison generates better code in Swift 5.10.
if absolute.bitWidth <= Int.bitWidth {
if distance < Int.zero {
return distance
}
} else {
if absolute <= Self(truncatingIfNeeded: Int.min.magnitude) {
return distance
}
}
}
_preconditionFailure("Distance is not representable in Int")
}
}Edit: I suppose StdlibUnittest calls it |
This version drops the same-size signed integer debug message.
A branch condition was more complicated than it had to be.
|
@swift-ci please benchmark |
|
@swift-ci please test |
Performance (x86_64): -O
Code size: -O
Performance (x86_64): -Osize
Code size: -Osize
Performance (x86_64): -Onone
Code size: -swiftlibsHow to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Do you have any thoughts on the results? I would not have thought there would be specialized fixed-width regressions (stdlib). Although, I'm unfamiliar with the tests that regressed. String stuff, mostly. I could check what they are all about. I suppose I could add some [U]Int128 tests while I'm at it. |
|
Most results look like noise (especially those marked with a "?"). Though it would be good to double check the few regressions (not marked with "?"). E.g. by running the benchmarks in instruments and look at the disassembly differences in the hot code area |
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 patch fixes two unnecessary traps in BinaryInteger's distance(to:) method.
UInt.max.distance(to: UInt.max / 2), etc.Int64(Int.max).distance(to: Int64(-1 as Int)), etc.The new signed code path uses bit width comparisons instead of a value comparisons, which are constant-foldable at compile time for fixed-width integers. The unsigned code path now inlines something similar to Int.init(exactly:), but with appropriate comparisons when needed. I rewrote both unsigned code paths for symmetry.
Here's Int.init(exactly:) for reference:
https://github.com/apple/swift/blob/e0853ff796adba7fc05ddfc48933ede619c4f4a0/stdlib/public/core/Integers.swift#L3708-L3723
Before
After
Addendum
This patch is the least complicated solution I came up with, and it does not preserve the error message for signed integers. I could alter the implementation, if that is important (with some performance penalty when self.bitWidth > Int.bitWidth). I thought I'd leave this part up for review, however, given that Swift.Int only logs overflow:
https://github.com/apple/swift/blob/e0853ff796adba7fc05ddfc48933ede619c4f4a0/stdlib/public/core/IntegerTypes.swift.gyb#L1817-L1820