Skip to content

SemanticARCOpts fix - remove deadEndBlocks checks #40500

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 1 commit into from
Dec 10, 2021

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Dec 10, 2021

SemanticARCOptVisitor::performGuaranteedCopyValueOptimization was
converting this SIL

%borrow = begin_borrow %copiedValue
%copy = copy_value %borrow
%borrowCopy = begin_borrow %copy
end_borrow %borrow
end_borrow %borrowCopy
destroy_value %copy
// something something
unreachable

into

%borrow = begin_borrow %copiedValue
%innerBorrow = begin_borrow %borrow
end_borrow %borrow
end_borrow %innerBorrow
// something something
unreachable

Dead-end blocks are simply irrelevant for this
optimization. Unfortunately, there were multiple layers of attempted
workarounds that were hiding the real problem, except in rare cases.

Thanks Nate Chandler for reducing the test.


I have a working branch that removes dead-end blocks nonsense completely from OSSA and further cleans up the code. But this targeted bug fix is urgently needed.

SemanticARCOptVisitor::performGuaranteedCopyValueOptimization was
converting this SIL

    %borrow = begin_borrow %copiedValue
    %copy = copy_value %borrow
    %borrowCopy = begin_borrow %copy
    end_borrow %borrow
    end_borrow %borrowCopy
    destroy_value %copy
    // something something
    unreachable

into

    %borrow = begin_borrow %copiedValue
    %innerBorrow = begin_borrow %borrow
    end_borrow %borrow
    end_borrow %innerBorrow
    // something something
    unreachable

Dead-end blocks are simply irrelevant for this
optimization. Unfortunately, there were multiple layers of attempted
workarounds that were hiding the real problem, except in rare cases.

Thanks Nate Chandler for reducing the test.
@atrick
Copy link
Contributor Author

atrick commented Dec 10, 2021

@swift-ci test

@atrick atrick merged commit 849af19 into swiftlang:main Dec 10, 2021
@atrick atrick deleted the fix-semanticarc branch December 10, 2021 17:12
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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.

2 participants