Skip to content

[5.3] SILOptimizer: A new "TempLValueOpt" optimization for copy_addr #32962

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 4 commits into from
Jul 18, 2020

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Jul 17, 2020

Cherry-picking this PR to 5.3 to unblock performance for
#32041

…ntial_addr.

Look "through" those instructions when trying to find the underlying objects.

(cherry picked from commit c7c2c13)
Currently we only have load [take] in OSSA which needed to be changed.
(copy_addr is not handled in MemBehavior at all, yet)

Even if the memory is physically not modified, conceptually it's "destroyed" when the value is taken.
Optimizations, like TempRValueOpt rely on this behavior when the check for may-writes.

This fixes a MemoryLifetime failure in TempRValueOpt.

(cherry picked from commit 547d527)
Only let copy_addr have side effects if the source or destination really aliases with the address in question.

(cherry picked from commit c82f78b)
Optimizes copies from a temporary (an "l-value") to a destination.

    %temp = alloc_stack $Ty
    instructions_which_store_to %temp
    copy_addr [take] %temp to %destination
    dealloc_stack %temp

is optimized to

    destroy_addr %destination
    instructions_which_store_to %destination

The name TempLValueOpt refers to the TempRValueOpt pass, which performs a related transformation, just with the temporary on the "right" side.
The TempLValueOpt is similar to CopyForwarding::backwardPropagateCopy.
It's more restricted (e.g. the copy-source must be an alloc_stack).
That enables other patterns to be optimized, which backwardPropagateCopy cannot handle.

This pass also performs a small peephole optimization which simplifies copy_addr - destroy sequences.

    copy_addr %source to %destination
    destroy_addr %source

is replace with

    copy_addr [take] %source to %destination

(cherry picked from commit 9e92389)
@atrick atrick requested a review from a team as a code owner July 17, 2020 22:27
@atrick
Copy link
Contributor Author

atrick commented Jul 17, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Jul 17, 2020

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Jul 17, 2020

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ed2e6fb

@atrick
Copy link
Contributor Author

atrick commented Jul 18, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Jul 18, 2020

--- CCC ---
Explanation: Add a peephole optimization to remove temporary copies of enums with existential payloads.

Radar: rdar://65755048 (Add the TempLValue SIL optimization to the 5.3 swift release branch)

Require to unlock performance fixes from this PR:
#32041

Scope: At -O, removes copies of more structurally complex values.

Risk: The optimization is restricted to a very limited input pattern and I've carefully reviewed the code. It could expose bugs in AliasAnalysis, but more testing exposure to core SIL analyses is generally good.

Testing: New unit tests and existing compatibility tests were run.

Reviewer: Andrew Trick

@atrick
Copy link
Contributor Author

atrick commented Jul 18, 2020

Explanation:

TempLValueOpt is designed to handle this case.

public enum Either<Left, Right> {
  case left(Left), right(Right)
}

internal struct AddressOnlyPayload {
  let a: Any
  let i: Int
}

public struct TestStruct {
  internal var e: Either<AddressOnlyPayload, Int>

  public init(_ a: Any, _ i: Int) {
    // Construct Either in-place.
    e = Either.left(AddressOnlyPayload(a: a, i: i))
  }
}

TempLValueOpt is conceptually much simpler than CopyForwarding for two key reasons:

  • it relies on the presence of AliasAnalysis (a non-goal for CopyForwarding).

  • it only recognizes copies where the source lifetime is clearly
    disjoint from the destination by scanning a short sequence of
    instructions (when CopyForwarding was written, SILGen already
    handled trivial copies)

SILGen was changed over time to eliminate less copies and AliasAnalysis improved. In response, we added a simpler TempRValueElimination to handle obvious copies. By calling AliasAnalysis it is potentially expensive, but in a sense more general because it only needs to analyze the use-patterns of the temporary value, not the use-patterns of the copied value.

The new TempLValueOpt is the just the backward form of TempRValueElimination. It is considerably simpler even than TempRValueElimination because it assumes the alloc_stack and dealloc_stack instructions are in the same block as the copy.

@atrick
Copy link
Contributor Author

atrick commented Jul 18, 2020

@airspeedswift benchmark PR testing appears to be broken, but the benchmarks did at least build and run locally, so I'll go ahead and merge.

@atrick atrick merged commit 63f79b6 into swiftlang:release/5.3 Jul 18, 2020
@atrick atrick deleted the 5.3-templvalue branch August 4, 2020 20:24
@AnthonyLatsis AnthonyLatsis added swift 5.3 🍒 release cherry pick Flag: Release branch cherry picks labels Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 5.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants