-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Several optimization improvements, mainly for begin_borrow, load and load_borrow
#86113
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.
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
Conversation
08d411a to
516c10b
Compare
|
@swift-ci test |
|
@swift-ci apple silicon benchmark |
| return true | ||
| } | ||
|
|
||
| private func tryForwardStoreBorrow(_ context: SimplifyContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a unit test for store borrow forwarding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! I added a test and also a source comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I think this maybe the first transform assuming store_borrow result immutability within its scope. It maybe good to verify this like we verify load_borrow source immutability via verifyNoMutatingUsesInLiverange in the future..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It maybe good to verify this like we verify load_borrow source immutability via verifyNoMutatingUsesInLiverange in the future..
good idea!
| /// ``` | ||
| /// %1 = load [copy] %0 | ||
| /// %2 = unchecked_ref_cast %1 : $SomeClass to $OtherClass | ||
| /// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment why this transform is useful?
516c10b to
55963c0
Compare
|
@swift-ci test |
55963c0 to
05f115f
Compare
|
@swift-ci test |
| /// bb0(%0 : @guaranteed $T): | ||
| /// // ... uses of %0 | ||
| /// ``` | ||
| private func tryReplaceInnerBorrowScope(beginBorrow: BeginBorrowInst, _ context: SimplifyContext) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a lexical check here? Can we remove an inner lexical borrow with an outer non lexical borrow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lexical check is only done in the mandatory pipeline. So, when running in the optimization pipeline, lexical begin_borrrows will be removed, anyway.
Can we remove an inner lexical borrow with an outer non lexical borrow?
I don't think so (in the mandatory pipeline) because then the lexical flag gets lost and the outer, non-lexical, borrow scope could be removed as well
|
I have a another comment - #86113 (comment) Otherwise LGTM |
|
@swift-ci test linux |
877eded to
03617ea
Compare
|
@swift-ci test linux |
…orrow.swift and SimplifyLoadBorrow.swift
* rename `lookThroughSingleForwardingUses` -> `lookThroughOwnedConvertibaleForwardingChain` * fix the comment of `replaceGuaranteed`
When trying to remove a borrow scope by replacing all guaranteed uses with owned uses, don't bail for values which have debug_value uses. Also make sure that the debug_value instructions are located within the owned value's lifetime.
* remove borrow scopes which are borrowing an already guaranteed value * allow optimizing lexical `begin_borrows` outside the mandatory pipeline * fix: don't remove `begin_borrow [lexical]` of a `thin_to_thick_function` in the mandatory pipeline
…now covered by SimplifyBeginBorrow
…alue When taking the location from the builder's insertion point, we must make sure it's not a return location. Fixes an assertion failure. I found this during my work on simplifying borrow scopes
* Replace address casts of heap objects ``` %1 = unchecked_addr_cast %0 : $*SomeClass to $*OtherClass %2 = load [copy] %1 ``` with ref-casts of the loaded value ``` %1 = load [copy] %0 %2 = unchecked_ref_cast %1 : $SomeClass to $OtherClass ``` * Replace a `load_borrow` of a `store_borrow` with a `begin_borrow`: ``` %1 = alloc_stack $T %2 = store_borrow %0 to %1 ... %3 = load_borrow %2 // ... uses of %3 end_borrow %3 ``` -> ``` %1 = alloc_stack $T %2 = store_borrow %0 to %1 ... %3 = begin_borrow %0 // ... uses of %3 end_borrow %3 ```
… with guaranteed ownership to to `tuple_extract`/`struct_extract`
By defining its own array structs, which are independent from objc interop. Also, the requires-line was wrong anyway. The test didn't run on macos either.
…hen changes are made When ownership is changed from owned to guaranteed, the enclosing values of a guaranteed value can change. Fixes a SIL verifier error.
…AddPriorityEscalationHandler` builtins The operand is a closure which escapes to the return value to the builtin. Therefore the operand ownership must be `BitwiseEscape`. Otherwise the closure might be destroyed too early. Fixes a mis-compile in the Concurrency library.
Fixes a compiler crash in LoadableByAddress. Unfortunately I don't have a test case for this.
…ined SIL function This error was not printed because the verifier complained before the error handling was done.
The thing with `optional<VerifierErrorEmitterGuard>` didn't work as expected. This resulted in a null-pointer de-reference when printing a verifier error.
…conversion in OSSA In ownership converting an unowned bitwise cast to a "real" ownership forwarding instruction can cause various troubles. Fixes a SIL verifier crash.
03617ea to
a06733b
Compare
|
@swift-ci test |
|
@swift-ci smoke test macos |
debug_valueinbegin_borrowsimplificationbegin_borrowsoutside the mandatory pipelineload_borrowof astore_borrowwith abegin_borrowdestructure_tuple/destructure_structwith guaranteed ownership to totuple_extract/struct_extractThe improvements for
begin_borrowsimplification allows to remove the now obsolete BorrowScope optimization from SemanticArcOpts.This PR also fixes a few small bugs which I run into while working on the optimizations:
taskAddCancellationHandlerandtaskAddPriorityEscalationHandlerbuiltinsunchecked_bitwise_castinstructionbegin_borrow [lexical]of athin_to_thick_functionin the mandatory pipelinedestroy_valueunchecked_bitwise_cast->unchekced_ref_castconversion in OSSAFor details see the commit messages