Skip to content

[5.9] AliasAnalysis: add complexity limits to some basic utility functions #68327

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 2 commits into from
Sep 6, 2023

Conversation

eeckstein
Copy link
Contributor

  • Explanation: This fixes a compile time problem which shows up for large nested arrays. The underlying problem is a quadratic complexity in the ARCSequenceOpts. The fix (or workaround) is to add a complexity limit in one of the three AliasAnalysis basic utility functions (which traverse the def-use graph). We already added this kind of complexity limit in the first utility functions some time ago (and this is already in 5.9). This PR contains two commits which each add the complexity limit to one of the two other functions. The first commit was already merged to main some time ago, but unfortunately not cherry-picked to 5.9. The second commit fixes the new problem and depends on the first commit. Therefore it's required and makes sense to cherry-pick both commits.

  • Issue: rdar://114352817

  • Risk: Low. The change makes some optimizations more conservative and this only happens in very rare cases.

  • Testing: With a regression test.

  • Reviewer: @atrick

  • Main branch PR: 29246fd of Optimizer: re-implement the RedundantLoadElimination pass in Swift #67395 and AliasAnalysis: use a complexity limit for the isObjReleased function #68316

We already use a complexity limit for other functions in AliasAnalysis.
This is a workaround for quadratic complexity in ARCSequenceOpts.

Fixes a compile time problem
rdar://114352817
@eeckstein eeckstein requested a review from a team as a code owner September 5, 2023 17:23
@eeckstein eeckstein requested a review from atrick September 5, 2023 17:24
@eeckstein
Copy link
Contributor Author

@swift-ci test

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.

Reviewed for correctness.

It took me a quite while to figure out what this change really means.

Summary:

swift::mayDecrementRefCount is the basic primitive for all ARC optimization: ARCSequenceOpts and ARCCodeMotion. These passes are quadratic in the number of live objects. Each live object uses a separate call to mayDecrementRefCount which is implemented in terms of AliasAnalysis::isObjectReleasedByInst, which calls AliasAnalysis::getOwnershipEffect.

AliasAnalysis::getOwnershipEffect calls into escape analysis to walk the SSA graph. Escape analysis has a normal budget to limit the graph traversal.

This changes AliasAnalysis::isObjectReleasedByInst to use 1/10 the normal escape analysis budget. This only affects the ARC passes that call into this entry point.

The deeper, underlying bug is that AliasAnalysis::getOwnershipEffect does not support multiple simultaneous queries. All roots could be queried with a single escape analysis walk.

@eeckstein eeckstein merged commit c3553a7 into swiftlang:release/5.9 Sep 6, 2023
@eeckstein eeckstein deleted the fix-compiletime-bug branch September 6, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants