Skip to content

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.

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?

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