Skip to content

Fix SILCombine of pointer_to_address instruction #64533

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
Mar 22, 2023

Conversation

meg-gupta
Copy link
Contributor

This change ensures the insertion point of the unchecked_addr_cast is after any copies/borrows that OSSA rauw may create.

Before this fix we could end up creating invalid SIL where operands do not dominate its instruction.

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test

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

// CHECK-LABEL: sil [ossa] @a2p_p2a_reinterpret_cast_word_raw_pointer2
// CHECK: unchecked_addr_cast
// CHECK: } // end sil function 'a2p_p2a_reinterpret_cast_word_raw_pointer2'
sil [ossa] @a2p_p2a_reinterpret_cast_word_raw_pointer2 : $@convention(thin) (@owned ContiguousArray<UInt16>) -> () {
Copy link
Contributor

@nate-chandler nate-chandler Mar 22, 2023

Choose a reason for hiding this comment

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

Does this test currently fail? Running sil-opt -sil-combine on this function with a not too old main (70b982f), valid code seems to be generated even when specifying -sil-opt-pass-count=1.6 to stop after adding the unchecked_addr_cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I had minimized the test too much.. Modified to a test that will fail without the patch now.

This change ensures the insertion point of the unchecked_addr_cast is after
any copies/borrows that OSSA rauw may create.

Before this fix we could end up creating invalid SIL where operands do not
dominate its instruction.
@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 42c69cc into swiftlang:main Mar 22, 2023
meg-gupta added a commit to meg-gupta/swift that referenced this pull request Mar 22, 2023
This change ensures the insertion point is after any copies/borrows that OSSA rauw may create.

Before this fix we could end up creating invalid SIL where operands do not dominate its instruction.

Similar to swiftlang#64533. Found by inspection.
meg-gupta added a commit to meg-gupta/swift that referenced this pull request Mar 22, 2023
This change ensures the insertion point is after any copies/borrows that OSSA rauw may create.

Before this fix we could end up creating invalid SIL where operands do not dominate its instruction.

Similar to swiftlang#64533. Found by inspection.
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