Skip to content

Fix array bounds check optimization for ossa #78231

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 18, 2024

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Dec 17, 2024

While hoisting check_subscript call, isNativeTypeChecked call is also created in the preheater .
The array value used in the isNativeTypeChecked may not be available if its lifetime had ended before (ossa only). Proactively set the array value of the isNativeTypeChecked call to the array value in the check_subscript call.

rdar://141630349

@meg-gupta meg-gupta marked this pull request as ready for review December 17, 2024 05:52
@meg-gupta meg-gupta requested a review from eeckstein as a code owner December 17, 2024 05:52
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

Hoisting the bounds check operations should not move any uses below lifetimes ends.

Looking at your examples, the problem seems to be that the array value is replaced with the operand of a copy_value. If this is not done, the resulting OSSA should be fine.

While hoisting check_subscript call in ossa, isNativeTypeChecked call is also hoisted.
The array value used in the isNativeTypeChecked may not be available if it's lifetime
had ended before. Proactively set the array value of the isNativeTypeChecked call to
the array value in the check_subscript call.
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta meg-gupta enabled auto-merge December 17, 2024 19:40
@meg-gupta meg-gupta merged commit cda907e into swiftlang:main Dec 18, 2024
4 of 5 checks passed
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