Skip to content

Commit f2302b9

Browse files
authored
Merge pull request #63034 from xwu/62260-heterogeneous-integer-comparisons-produce-suboptimal-code
[stdlib] Improve performance of heterogeneous binary integer `==` and `<`
2 parents 20f51b2 + df6a56e commit f2302b9

File tree

3 files changed

+67
-80
lines changed

3 files changed

+67
-80
lines changed

stdlib/public/core/Integers.swift

Lines changed: 37 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1658,39 +1658,26 @@ extension BinaryInteger {
16581658
public static func == <
16591659
Other: BinaryInteger
16601660
>(lhs: Self, rhs: Other) -> Bool {
1661-
let lhsNegative = Self.isSigned && lhs < (0 as Self)
1662-
let rhsNegative = Other.isSigned && rhs < (0 as Other)
1663-
1664-
if lhsNegative != rhsNegative { return false }
1665-
1666-
// Here we know the values are of the same sign.
1667-
//
1668-
// There are a few possible scenarios from here:
1669-
//
1670-
// 1. Both values are negative
1671-
// - If one value is strictly wider than the other, then it is safe to
1672-
// convert to the wider type.
1673-
// - If the values are of the same width, it does not matter which type we
1674-
// choose to convert to as the values are already negative, and thus
1675-
// include the sign bit if two's complement representation already.
1676-
// 2. Both values are non-negative
1677-
// - If one value is strictly wider than the other, then it is safe to
1678-
// convert to the wider type.
1679-
// - If the values are of the same width, than signedness matters, as not
1680-
// unsigned types are 'wider' in a sense they don't need to 'waste' the
1681-
// sign bit. Therefore it is safe to convert to the unsigned type.
1682-
1683-
if lhs.bitWidth < rhs.bitWidth {
1684-
return Other(truncatingIfNeeded: lhs) == rhs
1685-
}
1686-
if lhs.bitWidth > rhs.bitWidth {
1687-
return lhs == Self(truncatingIfNeeded: rhs)
1661+
// Use bit pattern conversion to widen the comparand with smaller bit width.
1662+
if Self.isSigned == Other.isSigned {
1663+
return lhs.bitWidth >= rhs.bitWidth ?
1664+
lhs == Self(truncatingIfNeeded: rhs) :
1665+
Other(truncatingIfNeeded: lhs) == rhs
16881666
}
1689-
1690-
if Self.isSigned {
1691-
return Other(truncatingIfNeeded: lhs) == rhs
1667+
// If `Self` is signed but `Other` is unsigned, then we have to
1668+
// be a little more careful about widening, since:
1669+
// (1) a fixed-width signed type can't represent the largest values of
1670+
// a fixed-width unsigned type of equal bit width; and
1671+
// (2) an unsigned type (obviously) can't represent a negative value.
1672+
if Self.isSigned {
1673+
return lhs.bitWidth > rhs.bitWidth ? // (1)
1674+
lhs == Self(truncatingIfNeeded: rhs) :
1675+
(lhs >= (0 as Self) && Other(truncatingIfNeeded: lhs) == rhs) // (2)
16921676
}
1693-
return lhs == Self(truncatingIfNeeded: rhs)
1677+
// Analogous reasoning applies if `Other` is signed but `Self` is not.
1678+
return lhs.bitWidth < rhs.bitWidth ?
1679+
Other(truncatingIfNeeded: lhs) == rhs :
1680+
(rhs >= (0 as Other) && lhs == Self(truncatingIfNeeded: rhs))
16941681
}
16951682

16961683
/// Returns a Boolean value indicating whether the two given values are not
@@ -1730,34 +1717,26 @@ extension BinaryInteger {
17301717
/// - rhs: Another integer to compare.
17311718
@_transparent
17321719
public static func < <Other: BinaryInteger>(lhs: Self, rhs: Other) -> Bool {
1733-
let lhsNegative = Self.isSigned && lhs < (0 as Self)
1734-
let rhsNegative = Other.isSigned && rhs < (0 as Other)
1735-
if lhsNegative != rhsNegative { return lhsNegative }
1736-
1737-
if lhs == (0 as Self) && rhs == (0 as Other) { return false }
1738-
1739-
// if we get here, lhs and rhs have the same sign. If they're negative,
1740-
// then Self and Other are both signed types, and one of them can represent
1741-
// values of the other type. Otherwise, lhs and rhs are positive, and one
1742-
// of Self, Other may be signed and the other unsigned.
1743-
1744-
let rhsAsSelf = Self(truncatingIfNeeded: rhs)
1745-
let rhsAsSelfNegative = rhsAsSelf < (0 as Self)
1746-
1747-
1748-
// Can we round-trip rhs through Other?
1749-
if Other(truncatingIfNeeded: rhsAsSelf) == rhs &&
1750-
// This additional check covers the `Int8.max < (128 as UInt8)` case.
1751-
// Since the types are of the same width, init(truncatingIfNeeded:)
1752-
// will result in a simple bitcast, so that rhsAsSelf would be -128, and
1753-
// `lhs < rhsAsSelf` will return false.
1754-
// We basically guard against that bitcast by requiring rhs and rhsAsSelf
1755-
// to be the same sign.
1756-
rhsNegative == rhsAsSelfNegative {
1757-
return lhs < rhsAsSelf
1720+
// Use bit pattern conversion to widen the comparand with smaller bit width.
1721+
if Self.isSigned == Other.isSigned {
1722+
return lhs.bitWidth >= rhs.bitWidth ?
1723+
lhs < Self(truncatingIfNeeded: rhs) :
1724+
Other(truncatingIfNeeded: lhs) < rhs
17581725
}
1759-
1760-
return Other(truncatingIfNeeded: lhs) < rhs
1726+
// If `Self` is signed but `Other` is unsigned, then we have to
1727+
// be a little more careful about widening, since:
1728+
// (1) a fixed-width signed type can't represent the largest values of
1729+
// a fixed-width unsigned type of equal bit width; and
1730+
// (2) an unsigned type (obviously) can't represent a negative value.
1731+
if Self.isSigned {
1732+
return lhs.bitWidth > rhs.bitWidth ? // (1)
1733+
lhs < Self(truncatingIfNeeded: rhs) :
1734+
(lhs < (0 as Self) || Other(truncatingIfNeeded: lhs) < rhs) // (2)
1735+
}
1736+
// Analogous reasoning applies if `Other` is signed but `Self` is not.
1737+
return lhs.bitWidth < rhs.bitWidth ?
1738+
Other(truncatingIfNeeded: lhs) < rhs :
1739+
(rhs > (0 as Other) && lhs < Self(truncatingIfNeeded: rhs))
17611740
}
17621741

17631742
/// Returns a Boolean value indicating whether the value of the first

test/IRGen/integer-comparison.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// RUN: %target-swift-frontend -primary-file %s -O -emit-ir | %FileCheck %s
2+
// REQUIRES: CPU=x86_64 || CPU=arm64
3+
4+
func f(_ x: Int, _ y: UInt32) -> Bool { x < y }
5+
// CHECK-LABEL: define {{.*}} @"$s4main1fySbSi_s6UInt32VtF"(i64 %0, i32 %1)
6+
// CHECK: %2 = zext i32 %1 to i64
7+
// CHECK-NEXT: %3 = icmp sgt i64 %2, %0
8+
// CHECK-NEXT: ret i1 %3
9+
10+
func g(_ x: UInt32, _ y: Int) -> Bool { x < y }
11+
// CHECK-LABEL: define {{.*}} @"$s4main1gySbs6UInt32V_SitF"(i32 %0, i64 %1)
12+
// CHECK: %2 = zext i32 %0 to i64
13+
// CHECK-NEXT: %3 = icmp slt i64 %2, %1
14+
// CHECK-NEXT: ret i1 %3

test/IRGen/temporary_allocation/codegen.swift

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
// RUN: %target-swift-frontend -primary-file %s -O -emit-ir | %FileCheck %s --check-prefix=CHECK-%target-vendor
1+
// RUN: %target-swift-frontend -primary-file %s -O -emit-ir | %FileCheck %s --check-prefixes=CHECK,CHECK-LARGE-ALLOC,CHECK-LARGE-ALLOC-%target-vendor
2+
// RUN: %target-swift-frontend -primary-file %s -O -emit-ir | %FileCheck %s --check-prefix=CHECK-LARGE-STACK-ALLOC -DWORD=i%target-ptrsize
3+
// RUN: %target-swift-frontend -primary-file %s -O -emit-ir | %FileCheck %s --check-prefix=CHECK-LARGE-HEAP-ALLOC -DWORD=i%target-ptrsize
24

35
@_silgen_name("blackHole")
46
func blackHole(_ value: UnsafeMutableRawPointer?) -> Void
@@ -74,24 +76,16 @@ withUnsafeTemporaryAllocation(of: Void.self, capacity: 2) { buffer in
7476
withUnsafeTemporaryAllocation(byteCount: 0x0FFF_FFFF, alignment: 1) { buffer in
7577
blackHole(buffer.baseAddress)
7678
}
77-
// CHECK-apple: [[IS_OS_OK:%[0-9]+]] = call swiftcc i1 @"$ss26_stdlib_isOSVersionAtLeastyBi1_Bw_BwBwtF"
78-
// CHECK-apple: br i1 [[IS_OS_OK]], label %[[OS_OK_BR:[0-9]+]], label %[[UNSAFE_BR:[0-9]+]]
79-
80-
// CHECK-apple: [[UNSAFE_BR]]:
81-
// CHECK-unknown: [[UNSAFE_BR:[0-9]+]]:
82-
// CHECK: [[HEAP_PTR_RAW:%[0-9]+]] = call noalias i8* @swift_slowAlloc([[WORD]] 268435455, [[WORD]] -1)
83-
// CHECK: [[HEAP_PTR:%[0-9]+]] = ptrtoint i8* [[HEAP_PTR_RAW]] to [[WORD]]
84-
// CHECK: call swiftcc void @blackHole([[WORD]] [[HEAP_PTR]])
85-
// CHECK: call void @swift_slowDealloc(i8* [[HEAP_PTR_RAW]], [[WORD]] -1, [[WORD]] -1)
86-
87-
// CHECK-apple: [[OS_OK_BR]]:
88-
// CHECK: [[IS_SAFE:%[0-9]+]] = call zeroext i1 @swift_stdlib_isStackAllocationSafe([[WORD]] 268435455, [[WORD]] 1)
89-
// CHECK: br i1 [[IS_SAFE]], label %[[SAFE_BR:[0-9]+]], label %[[UNSAFE_BR]]
90-
91-
// CHECK: [[SAFE_BR]]:
92-
// CHECK: [[SPSAVE:%spsave[0-9]*]] = call i8* @llvm.stacksave()
93-
// CHECK: [[STACK_PTR_RAW:%temp_alloc[0-9]*]] = alloca [268435455 x i8], align 1
94-
// CHECK: [[STACK_PTR:%[0-9]+]] = ptrtoint [268435455 x i8]* [[STACK_PTR_RAW]] to [[WORD]]
95-
// CHECK: call swiftcc void @blackHole([[WORD]] [[STACK_PTR]])
96-
// CHECK: call void @llvm.stackrestore(i8* [[SPSAVE]])
97-
79+
// CHECK-LARGE-HEAP-ALLOC: [[HEAP_PTR_RAW:%[0-9]+]] = call noalias i8* @swift_slowAlloc([[WORD]] 268435455, [[WORD]] -1)
80+
// CHECK-LARGE-HEAP-ALLOC-NEXT: [[HEAP_PTR:%[0-9]+]] = ptrtoint i8* [[HEAP_PTR_RAW]] to [[WORD]]
81+
// CHECK-LARGE-HEAP-ALLOC-NEXT: call swiftcc void @blackHole([[WORD]] [[HEAP_PTR]])
82+
// CHECK-LARGE-HEAP-ALLOC-NEXT: call void @swift_slowDealloc(i8* [[HEAP_PTR_RAW]], [[WORD]] -1, [[WORD]] -1)
83+
84+
// CHECK-LARGE-STACK-ALLOC: [[STACK_PTR_RAW:%temp_alloc[0-9]*]] = alloca [268435455 x i8], align 1
85+
// CHECK-LARGE-STACK-ALLOC-NEXT: [[STACK_PTR:%[0-9]+]] = ptrtoint [268435455 x i8]* [[STACK_PTR_RAW]] to [[WORD]]
86+
// CHECK-LARGE-STACK-ALLOC-NEXT: call swiftcc void @blackHole([[WORD]] [[STACK_PTR]])
87+
88+
// CHECK-LARGE-ALLOC-DAG: [[IS_SAFE:%[0-9]+]] = call zeroext i1 @swift_stdlib_isStackAllocationSafe([[WORD]] 268435455, [[WORD]] 1)
89+
// CHECK-LARGE-ALLOC-DAG: br i1 [[IS_SAFE]], label %{{[0-9]+}}, label %{{[0-9]+}}
90+
// CHECK-LARGE-ALLOC-apple-DAG: [[IS_OS_OK:%[0-9]+]] = call swiftcc i1 @"$ss26_stdlib_isOSVersionAtLeastyBi1_Bw_BwBwtF"
91+
// CHECK-LARGE-ALLOC-apple-DAG: br i1 [[IS_OS_OK]], label %{{[0-9]+}}, label %{{[0-9]+}}

0 commit comments

Comments
 (0)