Skip to content

[SE-0322] Temporary uninitialized buffers #37666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2021
Merged

[SE-0322] Temporary uninitialized buffers #37666

merged 1 commit into from
Oct 25, 2021

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented May 27, 2021 •
edited
Loading

These changes add the ability to temporarily allocate buffers preferentially on the stack.

Pitch thread: https://forums.swift.org/t/pitch-temporary-uninitialized-buffers/48954
Evolution PR: swiftlang/swift-evolution#1379
Evolution thread: https://forums.swift.org/t/se-0322-temporary-uninitialized-buffers/51848

Resolves rdar://76198203.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - d33960386386929f871954271c79fe214d6b81f4

zoecarver
zoecarver reviewed May 28, 2021
View reviewed changes
@grynspan grynspan mentioned this pull request Jun 1, 2021
Merged
@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch 4 times, most recently from f978060 to d5614aa Compare June 2, 2021 19:30
@grynspan
Copy link
Contributor Author

grynspan commented Jun 2, 2021

@swift-ci please smoke test

@grynspan grynspan changed the title [WIP] withUnsafeUninitializedMutableBufferPointer() [SE-NNNN] Temporary uninitialized buffers Jun 2, 2021
varungandhi-apple
varungandhi-apple reviewed Jun 2, 2021
View reviewed changes
@varungandhi-apple
Copy link
Contributor

Please add some test cases under test/IRGen as well as test/stdlib.

varungandhi-apple
varungandhi-apple reviewed Jun 2, 2021
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Jun 2, 2021
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Jun 2, 2021
View reviewed changes
varungandhi-apple
varungandhi-apple reviewed Jun 2, 2021
View reviewed changes
@grynspan
Copy link
Contributor Author

grynspan commented Jun 2, 2021

Please add some test cases under test/IRGen as well as test/stdlib.

Yep, it's on my to-do list. :)

benrimmington
benrimmington reviewed Jun 4, 2021
View reviewed changes
zoecarver
zoecarver reviewed Jun 7, 2021
View reviewed changes
Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit these for now, but I have a few others to come.

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch 4 times, most recently from 2c71162 to 1ed6ed5 Compare June 11, 2021 13:54
benrimmington
benrimmington reviewed Jun 12, 2021
View reviewed changes
@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from 1ed6ed5 to 6874698 Compare August 19, 2021 14:36
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@jckarter
Copy link
Contributor

It's useful for high-level optimizations of the stack for us to keep stack allocations balanced with deallocation operations. Even if the deallocation doesn't concretely do anything at runtime, knowing when the lifetime of the stack slot ends means that LLVM can potentially reuse the stack memory for something else, reclaim dynamically-allocated stack memory (which we do in some cases with unspecialized generics), and make better choices when splitting coroutines and allocating context for async/read/modify allocations. So it'd be good to have a stackDealloc builtin to match the stackAlloc builtin; it could just emit an llvm.lifetime.end call for the allocation for now.

@grynspan
Copy link
Contributor Author

Makes sense @jckarter. This is something I'd add as a builtin and call inside the Swift implementation (defer { Builtin.stackDealloc(...) }), I take it?

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch 3 times, most recently from a41dabb to f41945b Compare August 27, 2021 23:35
@grynspan
Copy link
Contributor Author

@swift-ci please test linux

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from b0b23fa to 03310bd Compare October 21, 2021 04:40
@grynspan
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 03310bd6017d07cec2d0c28ab66cd62832f63bb4

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 03310bd6017d07cec2d0c28ab66cd62832f63bb4

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from c953299 to dc01f7b Compare October 21, 2021 12:42
@grynspan
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - dc01f7b7f0feaed8e703a0609b7fe4d875957284

@grynspan
Copy link
Contributor Author

Build failures are in unrelated tests.

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch 2 times, most recently from e017f08 to 6b8f6b2 Compare October 21, 2021 17:16
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from 6b8f6b2 to a343c77 Compare October 21, 2021 18:30
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
Copy link
Contributor Author

@swift-ci please smoke test windows

@grynspan grynspan requested review from lorentey and jckarter October 21, 2021 20:48
@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from a343c77 to 3b678cf Compare October 21, 2021 23:21
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

lorentey
lorentey approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from the stdlib side! 👍

I have one question about linkage below, to ensure we won't have back deployment issues.

@grynspan
Copy link
Contributor Author

@swift-ci please smoke test windows

@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from 3b678cf to 6894781 Compare October 22, 2021 14:53
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
[SE-0322] Temporary uninitialized buffers
f1bf7ba 
Adds two new IRGen-level builtins (one for allocating, the other for deallocating), a stdlib shim function for enhanced stack-promotion heuristics, and the proposed public stdlib functions.
@grynspan grynspan force-pushed the jgrynspan/temporary-buffers branch from 6894781 to f1bf7ba Compare October 25, 2021 15:20
@grynspan
Copy link
Contributor Author

@swift-ci please smoke test

@grynspan
Copy link
Contributor Author

@swift-ci please smoke test windows

@grynspan grynspan merged commit 687cee9 into swiftlang:main Oct 25, 2021
Azoy
Azoy reviewed Oct 25, 2021
View reviewed changes
test/stdlib/TemporaryAllocation.swift
func isStackAllocated(_ pointer: UnsafeRawPointer) -> Bool? {
var stackBegin: UInt = 0
var stackEnd: UInt = 0
if _swift_stdlib_getCurrentStackBounds(&stackBegin, &stackEnd) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorentey Does this test require availability? This _swift_stdlib_getCurrentStackBounds function doesn't exist in older OSes.

Copy link
Member

@lorentey lorentey Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! Yep, this should have an availability check and return nil if the fn isn't available.

Furthermore, I think we will need to add the weak_import attribute to this function, like swift_stdlib_isStackAllocationSafe. (Or add availability information to its declaration in the shims module.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary—these are testing new API that's only available with the new stdlib (although it can be emitted to older targets.) The tests won't even compile if the new symbols aren't available, let alone link.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case we're worried about is back deployment testing where the test is compiled with the latest stdlib but executed with a previous release. In that scenario, strongly linking to (and unconditionally calling) the new runtime entry point will trigger a failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. This PR is closed now but we can open another one for a fix.

@3405691582 3405691582 mentioned this pull request Nov 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xwu xwu xwu left review comments

@glessard glessard glessard left review comments

@lorentey lorentey lorentey approved these changes

@benrimmington benrimmington benrimmington left review comments

@Azoy Azoy Azoy left review comments

@zoecarver zoecarver zoecarver left review comments

@varungandhi-apple varungandhi-apple varungandhi-apple left review comments

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@jckarter jckarter Awaiting requested review from jckarter jckarter is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.