-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Optimizer: improve the load-copy-to-borrow optimization and implement it in swift #76926
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
@swift-ci test |
@swift-ci benchmark |
3f8ada0
to
ba6a618
Compare
@swift-ci test |
@swift-ci test macos |
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.
LGTM, modulo the question of completing lifetimes inside out.
|
||
/// Makes sure that the lifetime of `value` ends at all control flow paths, even in dead-end blocks. | ||
/// Inserts destroys in dead-end blocks if those are missing. | ||
func completeLifetime(of value: Value) { |
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 now a good time to add a parameter to control whether lifetimes end at the availability or lifetime boundary?
SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoadCopyToBorrowOptimization.swift
Outdated
Show resolved
Hide resolved
// ... // no destroy of %1! | ||
// unreachable // the lifetime of %1 ends here | ||
// | ||
context.completeLifetime(of: loadedValue) |
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.
Generally, lifetimes need to be completed inside out/bottom up (as in SILGenCleanup::completeOSSALifetimes
). I haven't come up with an example where that would be a problem in this usage, but I'm not confident it's not.
One way to do this would be to collect all the loads first. If any are found, complete all lifetimes inside out. Then determine which of the discovered loads are optimizable and optimize them.
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.
As discussed offline, I'm now using computeKnownLiveness
instead.
SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoadCopyToBorrowOptimization.swift
Outdated
Show resolved
Hide resolved
SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoadCopyToBorrowOptimization.swift
Outdated
Show resolved
Hide resolved
ba6a618
to
ab75d8b
Compare
@swift-ci test |
…nWorklist` To be used for specific instruction types
…utility Implemented by bridging the OSSALifetimeCompletion utility
… incomplete liveranges If `visitInnerUses ` is true, it's not assumed that inner scopes are linear. It forces to visit all interior uses if inner scopes.
… it in swift The optimization replaces a `load [copy]` with a `load_borrow` if possible. ``` %1 = load [copy] %0 // no writes to %0 destroy_value %1 ``` -> ``` %1 = load_borrow %0 // no writes to %0 end_borrow %1 ``` The new implementation uses alias-analysis (instead of a simple def-use walk), which is much more powerful. rdar://115315849
ab75d8b
to
eed8645
Compare
@swift-ci test |
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.
Observations and future suggestions...
--- Liverange::collectUses ---
InteriorUseWalker::init
does not actually need visitInnerUses: Bool
because it could just have a visitInnerUses
entry point that does:
innerScopeHandler = { return visitUsesOfOuter(value: $0) }
Liverange::collectUses
is just doing what I would call "forwarding-liveness". We could standardize that: basically a combination of computeLinearLiveness
and ForwardingDefUseWalker
.
findAdditionalUsesInDeadEndBlocks
can be eliminated with complete OSSA... ok, I just noticed a TODO says the same thing. But until then, it seems like it might be as efficient to call completeLife(of:)
first for each owned values, then simply look for endsLifetime
uses. Note that you do need to handle ExtendLifetimeInst
, just like computeLinearLiveness
does.
--- LoadBorrowInst::verify ---
Small note on load-borrow-immutability verification:
At some point, I would like to centralize the definition of legal address uses in one place. So isMutatedAddress
would be alongside AddressUseVisitor::classifyAddress
. Also note that, unlike the incorrectly named AddressDefUseWalker::leafUse
, AddressUseVisitor::leafAddressUse
is actually a leaf use, so you could probably just check mayWrite
instead of switching over opcodes.
Then verification should be improved so it does not ignore "unknown" uses (although in this particular verification, we always need to ignore known escaping uses).
👍. Though the drawback is that this is an escaping closure and needs memory allocation for its context. It's potentially called many times (for each load). So this might affect compile time.
That doesn't work because |
The optimization replaces a
load [copy]
with aload_borrow
if possible.->
The new implementation uses alias-analysis (instead of a simple def-use walk), which is much more powerful.
Also, implement load-borrow-immutability checkin in the swift verifier.
rdar://115315849