[stdlib] Resolve unsafeBitCast warnings in Runtime.swift.gyb. #7116
Conversation
|
@swift-ci Please benchmark |
There was a problem hiding this comment.
It's a good cleanup. I'm concerned about adding withMemoryRebound closures to atomic operations. It's not the sort of place we want heap allocation. In the atomicFetch cast, just use assumingMemoryBound(to:). Int is an Int64. I guess we can ignore the performance of the generic entry point unless the benchmark results look bad.
stdlib/public/core/Runtime.swift.gyb
Outdated
| return _stdlib_atomicCompareExchangeStrongPtr( | ||
| object: target, | ||
| expected: expected, | ||
| desired: UnsafeRawPointer(desired)) |
There was a problem hiding this comment.
I'm not sure why desired was originally bitcast, nor why it needs a seemingly redundant initializer call here.
There was a problem hiding this comment.
Purely style. I don't like relying on pointer-to-pointer conversions in pure Swift code.
|
Mandatory inlining should actually get rid of the closure. But still, assumingMemoryBound(to:) is perfectly fine for UnsafePointer -> UnsafePointer. And in general it's fine if the pointer is only used inside a C function. |
|
There's no assumingMemoryBound on the typed pointers, which makes it a little more annoying, but I can switch to that if you prefer. |
Build comment file:Optimized (O) Regression (1)
Improvement (0)No Changes (158)
Regression (0)Improvement (6)
No Changes (153)
|
|
In my mind, an atomic increment is more primitive than a closure taking function. I don't like unnecessary abstraction in the implementation of language primitives. I've seen too much SIL code and waiting too long for builds. I confess, I was thinking that It's still safe to use |
|
Also, none of this is meant to be pretty or convenient. I wouldn't worry about elegance or best practices at this level. Yes, we should for stdlib data structures, but this is just a builtin that happens to be implementable in Swift. |
jrose-apple
force-pushed
the
unsafeBitCast-Runtime.swift.gyb
branch
from
9fd0298 to
6501834
Compare
|
Updated. @atrick, is this what you were thinking? Are all of these uses safe? (Compare with the previous version: jrose-apple/swift@9fd0298) |
|
@swift-ci Please smoke test |
|
Yes, I'll stand by the correctness of this patch... I had to think about it overnight. Your original patch was generally the right way to handle these casts. But since this is what I call a "language primitive" it's a good place to explore the boundaries of manual optimization, both as a performance guarantee and because adding abstraction in "inlineable" primitives could conceivably hurt build time. I was arguing that The rule in the compiler should actually fall out of this logic:
I was not originally suggesting you change the other cases, but as long as we're being maximally aggressive in the atomics implementation... I also claim that we should explicitly call out
I suggest we call out
|
|
Can we sidestep the memory binding questions by having the |
|
It depends on how clear you want the wrapper API's to be. To use The @jrose-apple can decide if that's worth it. Or just hand it to me if you're bored of this. |
No intended functionality change.
jrose-apple
force-pushed
the
unsafeBitCast-Runtime.swift.gyb
branch
from
6501834 to
498afb8
Compare
|
I tried for a moment and it didn't look like it was going to go very well due to the need to load from and store to @swift-ci Please smoke test |
|
@jrose-apple I won't get to this today so I'll go ahead and push your change to fix the warning. I made a note to come back to it later. Thanks! |
No intended functionality change.