Skip to content

Commit 89cefe0

Browse files
gottesmmgottesmm
committed
[stdlib] Use a different access pattern to check uniqueness to work around more conservative SILGen codegen.
Using && here causes us to go down a SILGen path that guarantees that self will be evaluated over the entire && expression instead of just the LHS. This cause the uniqueness check to always return false at -Onone. At -O, the optimizer is smart enough to remove this issue. rdar://33358110
1 parent cf949cb commit 89cefe0

File tree

8 files changed

+175
-99
lines changed

8 files changed

+175
-99
lines changed

‎stdlib/public/core/ArrayBuffer.swift

+20-2
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,16 @@ extension _ArrayBuffer {
120120
if !_isClassOrObjCExistential(Element.self) {
121121
return _storage.isUniquelyReferenced_native_noSpareBits()
122122
}
123-
return _storage.isUniquelyReferencedNative() && _isNative
123+
124+
// This is a performance optimization. This code used to be:
125+
//
126+
// return _storage.isUniquelyReferencedNative() && _isNative.
127+
//
128+
// SR-6437
129+
if !_storage.isUniquelyReferencedNative() {
130+
return false
131+
}
132+
return _isNative
124133
}
125134

126135
/// Returns `true` iff this buffer's storage is either
@@ -131,7 +140,16 @@ extension _ArrayBuffer {
131140
if !_isClassOrObjCExistential(Element.self) {
132141
return _storage.isUniquelyReferencedOrPinned_native_noSpareBits()
133142
}
134-
return _storage.isUniquelyReferencedOrPinnedNative() && _isNative
143+
144+
// This is a performance optimization. This code used to be:
145+
//
146+
// return _storage.isUniquelyReferencedOrPinnedNative() && _isNative.
147+
//
148+
// SR-6437
149+
if !_storage.isUniquelyReferencedOrPinnedNative() {
150+
return false
151+
}
152+
return _isNative
135153
}
136154

137155
/// Convert to an NSArray.

‎stdlib/public/core/Arrays.swift.gyb

+10-1
Original file line numberDiff line numberDiff line change
@@ -1363,6 +1363,15 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol {
13631363
@_versioned
13641364
@_semantics("array.mutate_unknown")
13651365
internal mutating func _reserveCapacityAssumingUniqueBuffer(oldCount: Int) {
1366+
// This is a performance optimization. This code used to be in an ||
1367+
// statement in the _sanityCheck below.
1368+
//
1369+
// _sanityCheck(_buffer.capacity == 0 ||
1370+
// _buffer.isMutableAndUniquelyReferenced())
1371+
//
1372+
// SR-6437
1373+
let capacity = _buffer.capacity == 0
1374+
13661375
// Due to make_mutable hoisting the situation can arise where we hoist
13671376
// _makeMutableAndUnique out of loop and use it to replace
13681377
// _makeUniqueAndReserveCapacityIfNotUnique that preceeds this call. If the
@@ -1372,7 +1381,7 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol {
13721381
// This specific case is okay because we will make the buffer unique in this
13731382
// function because we request a capacity > 0 and therefore _copyToNewBuffer
13741383
// will be called creating a new buffer.
1375-
_sanityCheck(_buffer.capacity == 0 ||
1384+
_sanityCheck(capacity ||
13761385
_buffer.isMutableAndUniquelyReferenced())
13771386

13781387
if _slowPath(oldCount + 1 > _buffer.capacity) {

‎stdlib/public/core/HashedCollections.swift.gyb

+110-36
Original file line numberDiff line numberDiff line change
@@ -4880,51 +4880,88 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
48804880
}
48814881
#endif
48824882

4883-
/// Ensure this we hold a unique reference to a native buffer
4884-
/// having at least `minimumCapacity` elements.
4883+
/// Return true if self is native.
48854884
@_inlineable // FIXME(sil-serialize-all)
48864885
@_versioned // FIXME(sil-serialize-all)
4887-
internal mutating func ensureUniqueNativeBuffer(_ minimumCapacity: Int)
4888-
-> (reallocated: Bool, capacityChanged: Bool) {
4886+
internal var _isNative : Bool {
4887+
#if _runtime(_ObjC)
48894888
switch self {
48904889
case .native:
4891-
let oldCapacity = asNative.capacity
4892-
if isUniquelyReferenced() && oldCapacity >= minimumCapacity {
4893-
return (reallocated: false, capacityChanged: false)
4894-
}
4890+
return true
4891+
case .cocoa:
4892+
return false
4893+
}
4894+
#else
4895+
return true
4896+
#endif
4897+
}
48954898

4896-
let oldNativeBuffer = asNative
4897-
var newNativeBuffer = NativeBuffer(minimumCapacity: minimumCapacity)
4898-
let newCapacity = newNativeBuffer.capacity
4899-
for i in 0..<oldCapacity {
4900-
if oldNativeBuffer.isInitializedEntry(at: i) {
4901-
if oldCapacity == newCapacity {
4902-
let key = oldNativeBuffer.key(at: i)
4899+
@inline(__always)
4900+
@_inlineable // FIXME(sil-serialize-all)
4901+
@_versioned // FIXME(sil-serialize-all)
4902+
internal mutating func ensureUniqueNativeBufferNative(_ minimumCapacity: Int)
4903+
-> (reallocated: Bool, capacityChanged: Bool) {
4904+
let oldCapacity = asNative.capacity
4905+
if oldCapacity >= minimumCapacity && isUniquelyReferenced() {
4906+
return (reallocated: false, capacityChanged: false)
4907+
}
4908+
4909+
let oldNativeBuffer = asNative
4910+
var newNativeBuffer = NativeBuffer(minimumCapacity: minimumCapacity)
4911+
let newCapacity = newNativeBuffer.capacity
4912+
for i in 0..<oldCapacity {
4913+
if oldNativeBuffer.isInitializedEntry(at: i) {
4914+
if oldCapacity == newCapacity {
4915+
let key = oldNativeBuffer.key(at: i)
49034916
%if Self == 'Set':
4904-
newNativeBuffer.initializeKey(key, at: i)
4917+
newNativeBuffer.initializeKey(key, at: i)
49054918
%elif Self == 'Dictionary':
4906-
let value = oldNativeBuffer.value(at: i)
4907-
newNativeBuffer.initializeKey(key, value: value , at: i)
4919+
let value = oldNativeBuffer.value(at: i)
4920+
newNativeBuffer.initializeKey(key, value: value , at: i)
49084921
%end
4909-
} else {
4910-
let key = oldNativeBuffer.key(at: i)
4922+
} else {
4923+
let key = oldNativeBuffer.key(at: i)
49114924
%if Self == 'Set':
4912-
newNativeBuffer.unsafeAddNew(key: key)
4925+
newNativeBuffer.unsafeAddNew(key: key)
49134926
%elif Self == 'Dictionary':
4914-
newNativeBuffer.unsafeAddNew(
4915-
key: key,
4916-
value: oldNativeBuffer.value(at: i))
4927+
newNativeBuffer.unsafeAddNew(
4928+
key: key,
4929+
value: oldNativeBuffer.value(at: i))
49174930
%end
4918-
}
49194931
}
49204932
}
4921-
newNativeBuffer.count = oldNativeBuffer.count
4933+
}
4934+
newNativeBuffer.count = oldNativeBuffer.count
49224935

4923-
self = .native(newNativeBuffer)
4924-
return (reallocated: true,
4925-
capacityChanged: oldCapacity != newNativeBuffer.capacity)
4936+
self = .native(newNativeBuffer)
4937+
return (reallocated: true,
4938+
capacityChanged: oldCapacity != newNativeBuffer.capacity)
4939+
}
49264940

4941+
/// Ensure this we hold a unique reference to a native buffer
4942+
/// having at least `minimumCapacity` elements.
4943+
@_inlineable // FIXME(sil-serialize-all)
4944+
@_versioned // FIXME(sil-serialize-all)
4945+
internal mutating func ensureUniqueNativeBuffer(_ minimumCapacity: Int)
4946+
-> (reallocated: Bool, capacityChanged: Bool) {
49274947
#if _runtime(_ObjC)
4948+
// This is a performance optimization that was put in to ensure that we did
4949+
// not make a copy of self to call _isNative over the entire if region
4950+
// causing at -Onone the uniqueness check to fail. This code used to be:
4951+
//
4952+
// if _isNative {
4953+
// return ensureUniqueNativeBufferNative(minimumCapacity)
4954+
// }
4955+
//
4956+
// SR-6437
4957+
let n = _isNative
4958+
if n {
4959+
return ensureUniqueNativeBufferNative(minimumCapacity)
4960+
}
4961+
4962+
switch self {
4963+
case .native:
4964+
fatalError("This should have been handled earlier")
49284965
case .cocoa(let cocoaBuffer):
49294966
let cocoa${Self} = cocoaBuffer.cocoa${Self}
49304967
var newNativeBuffer = NativeBuffer(minimumCapacity: minimumCapacity)
@@ -4948,8 +4985,10 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
49484985

49494986
self = .native(newNativeBuffer)
49504987
return (reallocated: true, capacityChanged: true)
4951-
#endif
49524988
}
4989+
#else
4990+
return ensureUniqueNativeBufferNative(minimumCapacity)
4991+
#endif
49534992
}
49544993

49554994
#if _runtime(_ObjC)
@@ -5236,8 +5275,17 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
52365275
@_inlineable // FIXME(sil-serialize-all)
52375276
@_versioned // FIXME(sil-serialize-all)
52385277
internal mutating func nativePointerToValue(at i: Index)
5239-
-> UnsafeMutablePointer<Value> {
5240-
_ = ensureUniqueNativeBuffer(asNative.capacity)
5278+
-> UnsafeMutablePointer<Value> {
5279+
// This is a performance optimization that was put in to ensure that we did
5280+
// not make a copy of self to call asNative.capacity over
5281+
// ensureUniqueNativeBefore causing at -Onone the uniqueness check to
5282+
// fail. This code used to be:
5283+
//
5284+
// _ = ensureUniqueNativeBuffer(capacity)
5285+
//
5286+
// SR-6437
5287+
let capacity = asNative.capacity
5288+
_ = ensureUniqueNativeBuffer(capacity)
52415289
return asNative.values + i._nativeIndex.offset
52425290
}
52435291

@@ -5391,7 +5439,16 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
53915439
var (i, found) = asNative._find(key, startBucket: asNative._bucket(key))
53925440

53935441
if found {
5394-
_ = ensureUniqueNativeBuffer(asNative.capacity)
5442+
// This is a performance optimization that was put in to ensure that we
5443+
// did not make a copy of self to call asNative.capacity over
5444+
// ensureUniqueNativeBefore causing at -Onone the uniqueness check to
5445+
// fail. This code used to be:
5446+
//
5447+
// _ = ensureUniqueNativeBuffer(asNative.capacity)
5448+
//
5449+
// SR-6437
5450+
let capacity = asNative.capacity
5451+
_ = ensureUniqueNativeBuffer(capacity)
53955452
do {
53965453
let newValue = try combine(asNative.value(at: i.offset), value)
53975454
asNative.setKey(key, value: newValue, at: i.offset)
@@ -5545,8 +5602,17 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
55455602
return nil
55465603
}
55475604

5605+
// This is a performance optimization that was put in to ensure that we
5606+
// did not make a copy of self to call asNative.capacity over
5607+
// ensureUniqueNativeBefore causing at -Onone the uniqueness check to
5608+
// fail. This code used to be:
5609+
//
5610+
// _ = ensureUniqueNativeBuffer(asNative.capacity)
5611+
//
5612+
// SR-6437
5613+
let capacity = asNative.capacity
55485614
let (_, capacityChanged) =
5549-
ensureUniqueNativeBuffer(asNative.capacity)
5615+
ensureUniqueNativeBuffer(capacity)
55505616
let nativeBuffer = asNative
55515617
if capacityChanged {
55525618
idealBucket = nativeBuffer._bucket(key)
@@ -5568,10 +5634,18 @@ internal enum _Variant${Self}Buffer<${TypeParametersDecl}> : _HashBuffer {
55685634
internal mutating func nativeRemove(
55695635
at nativeIndex: NativeIndex
55705636
) -> SequenceElement {
5571-
5637+
// This is a performance optimization that was put in to ensure that we did
5638+
// not make a copy of self to call asNative.capacity over
5639+
// ensureUniqueNativeBefore causing at -Onone the uniqueness check to
5640+
// fail. This code used to be:
5641+
//
5642+
// _ = ensureUniqueNativeBuffer(asNative.capacity)
5643+
//
5644+
// SR-6437
5645+
let capacity = asNative.capacity
55725646
// The provided index should be valid, so we will always mutating the
55735647
// set buffer. Request unique buffer.
5574-
_ = ensureUniqueNativeBuffer(asNative.capacity)
5648+
_ = ensureUniqueNativeBuffer(capacity)
55755649
let nativeBuffer = asNative
55765650

55775651
let result = nativeBuffer.assertingGet(nativeIndex)

‎stdlib/public/core/SliceBuffer.swift

+35-4
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ internal struct _SliceBuffer<Element>
105105
_invariantCheck()
106106
_sanityCheck(insertCount <= numericCast(newValues.count))
107107

108-
_sanityCheck(_hasNativeBuffer && isUniquelyReferenced())
108+
_sanityCheck(_hasNativeBuffer)
109+
_sanityCheck(isUniquelyReferenced())
109110

110111
let eraseCount = subrange.count
111112
let growth = insertCount - eraseCount
@@ -167,7 +168,17 @@ internal struct _SliceBuffer<Element>
167168
minimumCapacity: Int
168169
) -> NativeBuffer? {
169170
_invariantCheck()
170-
if _fastPath(_hasNativeBuffer && isUniquelyReferenced()) {
171+
// This is a performance optimization that was put in to ensure that at
172+
// -Onone, copy of self we make to call _hasNativeBuffer is destroyed before
173+
// we call isUniquelyReferenced. Otherwise, isUniquelyReferenced will always
174+
// fail causing us to always copy.
175+
//
176+
// if _fastPath(_hasNativeBuffer && isUniquelyReferenced) {
177+
//
178+
// SR-6437
179+
let native = _hasNativeBuffer
180+
let unique = isUniquelyReferenced()
181+
if _fastPath(native && unique) {
171182
if capacity >= minimumCapacity {
172183
// Since we have the last reference, drop any inaccessible
173184
// trailing elements in the underlying storage. That will
@@ -195,13 +206,33 @@ internal struct _SliceBuffer<Element>
195206
@_inlineable
196207
@_versioned
197208
internal mutating func isMutableAndUniquelyReferenced() -> Bool {
198-
return _hasNativeBuffer && isUniquelyReferenced()
209+
// This is a performance optimization that ensures that the copy of self
210+
// that occurs at -Onone is destroyed before we call
211+
// isUniquelyReferencedOrPinned. This code used to be:
212+
//
213+
// return _hasNativeBuffer && isUniquelyReferenced()
214+
//
215+
// SR-6437
216+
if !_hasNativeBuffer {
217+
return false
218+
}
219+
return isUniquelyReferenced()
199220
}
200221

201222
@_inlineable
202223
@_versioned
203224
internal mutating func isMutableAndUniquelyReferencedOrPinned() -> Bool {
204-
return _hasNativeBuffer && isUniquelyReferencedOrPinned()
225+
// This is a performance optimization that ensures that the copy of self
226+
// that occurs at -Onone is destroyed before we call
227+
// isUniquelyReferencedOrPinned. This code used to be:
228+
//
229+
// return _hasNativeBuffer && isUniquelyReferencedOrPinned()
230+
//
231+
// SR-6437
232+
if !_hasNativeBuffer {
233+
return false
234+
}
235+
return isUniquelyReferencedOrPinned()
205236
}
206237

207238
/// If this buffer is backed by a `_ContiguousArrayBuffer`

‎test/stdlib/Inputs/CommonArrayTests.gyb

-12
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,6 @@ ${Suite}.test("${ArrayType}/Sliceable/Enums") {
102102
}
103103

104104
${Suite}.test("${ArrayType}/appendNonUnique")
105-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
106-
reason: "rdar://33358110"))
107105
.code {
108106
var x: ${ArrayType}<Int> = []
109107
x.reserveCapacity(10002)
@@ -269,8 +267,6 @@ let withUnsafeMutableBufferPointerIfSupportedTests = [
269267
]
270268

271269
${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported")
272-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
273-
reason: "rdar://33358110"))
274270
.code {
275271
for test in withUnsafeMutableBufferPointerIfSupportedTests {
276272
var a = getFresh${ArrayType}(test.sequence.map(OpaqueValue.init))
@@ -306,8 +302,6 @@ ${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported")
306302
}
307303

308304
${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported/ReplacingTheBufferTraps/1")
309-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
310-
reason: "rdar://33358110"))
311305
.code {
312306
var a = getFresh${ArrayType}([ OpaqueValue(10) ])
313307
var result = a._withUnsafeMutableBufferPointerIfSupported {
@@ -320,8 +314,6 @@ ${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported/Replacing
320314
}
321315

322316
${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported/ReplacingTheBufferTraps/2")
323-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
324-
reason: "rdar://33358110"))
325317
.code {
326318
var a = getFresh${ArrayType}([ OpaqueValue(10) ])
327319
var result = a._withUnsafeMutableBufferPointerIfSupported {
@@ -339,8 +331,6 @@ ${Suite}.test("${ArrayType}/_withUnsafeMutableBufferPointerIfSupported/Replacing
339331

340332
// Test the uniqueness of the raw buffer.
341333
${Suite}.test("${ArrayType}/withUnsafeMutableBytes")
342-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
343-
reason: "rdar://33358110"))
344334
.code {
345335
var a = getFresh${ArrayType}([UInt8](repeating: 10, count: 1))
346336
let b = a
@@ -365,8 +355,6 @@ ${Suite}.test(
365355

366356
${Suite}.test(
367357
"${ArrayType}/mutationDoesNotAffectIterator/subscript/append")
368-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${ArrayType}" == "ArraySlice"},
369-
reason: "rdar://33358110"))
370358
.code {
371359
var arr: ${ArrayType}<Int> = [ 1010, 1020, 1030 ]
372360
var iter = arr.makeIterator()

‎validation-test/stdlib/ArrayNew.swift.gyb

-2
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,6 @@ func withInoutT<T>(_ x: inout T, body: (_ x: inout T) -> Void) {
4949
% end
5050

5151
ArrayTestSuite.test("${array_type}<${element_type}>/subscript(_: Int)/COW")
52-
.xfail(.custom({ _isStdlibDebugConfiguration() && "${array_type}" == "ArraySlice" },
53-
reason: "rdar://33358110"))
5452
.code {
5553
var a: ${array_type}<${array_type}<${element_type}>> = [[
5654
${element_type}(10), ${element_type}(20), ${element_type}(30),

0 commit comments

Comments
 (0)