Skip to content

[CSE] Stack nest at OSSA lowering after inlining. #64005

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

Conversation

nate-chandler
Copy link
Contributor

CSE inlines a portion of lazy property getters.

Now that in OSSA partial_apply [on_stack]s are represented as owned values rather than stack locations, it is possible for their destroys to violate stack discipline. A direct lowering of the instructions to non-OSSA would violate stack nesting.

Previously, when inlining during CSE, it was assumed that the callee maintained stack discipline. And, when inlining an OSSA function into a non-OSSA function, OSSA instructions were lowered directly. The result was that stack discipline could be violated when directly lowering callees with partial_apply [on_stack]s that violate stack discipline upon direct lowering.

Here, when CSE inlining a lazy property getter in OSSA form into a function lowered out of OSSA form, stack nesting is fixed up.

CSE inlines a portion of lazy property getters.

Now that in OSSA `partial_apply [on_stack]`s are represented as owned
values rather than stack locations, it is possible for their destroys to
violate stack discipline.  A direct lowering of the instructions to
non-OSSA would violate stack nesting.

Previously, when inlining during CSE, it was assumed that the callee
maintained stack discipline.  And, when inlining an OSSA function into a
non-OSSA function, OSSA instructions were lowered directly.  The result
was that stack discipline could be violated when directly lowering
callees with `partial_apply [on_stack]`s that violate stack discipline
upon direct lowering.

Here, when CSE inlining a lazy property getter in OSSA form into a
function lowered out of OSSA form, stack nesting is fixed up.
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@meg-gupta meg-gupta left a comment

Choose a reason for hiding this comment

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

LGTM!

@nate-chandler nate-chandler merged commit 7ba396a into swiftlang:main Mar 2, 2023
@nate-chandler nate-chandler deleted the cse/partial-apply-stack-discipline branch March 2, 2023 02:09
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.

@nate-chandler looks great to me too.

Just an observation. It might be less bug prone for the inliner to do this work itself. For efficiency, we might then want fixNesting to work on a region rather than the whole function.

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