Skip to content

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

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

eeckstein
Copy link
Contributor

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.
Also, implement load-borrow-immutability checkin in the swift verifier.

rdar://115315849

@eeckstein eeckstein requested a review from jckarter as a code owner October 9, 2024 08:16
@eeckstein eeckstein requested review from meg-gupta and removed request for jckarter October 9, 2024 08:16
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@swift-ci benchmark

@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@swift-ci test macos

Copy link
Contributor

@nate-chandler nate-chandler left a 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) {
Copy link
Contributor

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?

// ... // no destroy of %1!
// unreachable // the lifetime of %1 ends here
//
context.completeLifetime(of: loadedValue)
Copy link
Contributor

@nate-chandler nate-chandler Oct 9, 2024

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.

Copy link
Contributor Author

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.

@eeckstein
Copy link
Contributor Author

@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
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein eeckstein merged commit c06a9e5 into swiftlang:main Oct 11, 2024
5 checks passed
@eeckstein eeckstein deleted the load-copy-opt branch October 11, 2024 13:20
Copy link
Contributor

@atrick atrick left a 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).

@eeckstein
Copy link
Contributor Author

InteriorUseWalker::init does not actually need visitInnerUses: Bool because it could just have a visitInnerUses entry point that does:

👍. 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.

so you could probably just check mayWrite instead of switching over opcodes.

That doesn't work because mayWrite is less precise than what the load-copy-to-borrow optimization does using alias analysis. It would result in false alarms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants