Skip to content

6.2: [TypeLowering] Record pack used in aggregate signature. #82044

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

@nate-chandler nate-chandler commented Jun 6, 2025

Explanation: Fix an assertion failure involving a generic, non-variadic-generic, type into which a variadic generic type is substituted.

Metadata packs are stored on the stack as an optimization. These allocations must obey stack discipline. This is enforced via the PackMetadataMarkerInserter pass. That pass inserts alloc_pack_metadata marker instructions before each instruction which may allocate an on-stack pack and inserts dealloc_pack_metadata instructions to indicate where those packs should be deallocated (the StackNesting utility ensures proper nesting, reflowing all stack deallocating instructions as needed). The pass relies on knowing whether an instruction may allocate an on-stack pack. This is determined by a recursive walk over the fields of each type of each operand of the instruction. That walk is done in TypeLowering.

Prior to #81659 , whether a type involved a pack in its signature was not considered by TypeLowering. Only whether the outermost type's signature involved a pack was considered by the typeOrLayoutInvolvesPack helper function called by the pass. This was incorrect. After that PR, whether any type visited recursively involved a pack in its signature was considered. This was achieved by adding merging the pack-ness of each intermediate and leaf type's signature into the outermost type's pack-ness. Specifically, this merging was done in each LowerType::handle* in TypeLowering.cpp--these functions are the final step in computing each type's properties, so one will be called for every single type that's visited along the way.

But I missed one of the LowerType::handle* functions. This in combination with the removal (correct) of the check in typeOrLayoutInvolvesPack of whether the outermost type involved a pack in its signature led to a regression in certain cases such as that in the included test case.

The solution, implemented here, is to merge in the pack-ness in the one LowerType::handle* function from which it was missing. This addresses the regression without reintroducing the wrong approach of checking the outermost type's signature which happened to work in some simple cases.
Scope: Variadic generics.
Issue: rdar://152580661
Original PR: #82043
Risk: Low, this completes the pattern set in the previous PR, ensuring that all types merge pack-ness from their signatures.
Testing: Added test.
Reviewer: Arnold Schawighofer ( @aschwaighofer)

Every `LowerType::visit*` function eventually calls through to a
`LowerType::handle*` function.  After
swiftlang#81581, every
`LowerType::handle*` needs to set the `hasPack` flag based on the
passed-in type by calling `mergeHasPack`.  Add the missing call in the
`handleAggregateByProperties` function.

rdar://152580661
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

@swift-ci please test macos platform

1 similar comment
@nate-chandler
Copy link
Contributor Author

@swift-ci please test macos platform

@nate-chandler nate-chandler marked this pull request as ready for review June 6, 2025 18:37
@nate-chandler nate-chandler requested a review from a team as a code owner June 6, 2025 18:37
@nate-chandler nate-chandler enabled auto-merge June 6, 2025 18:37
@nate-chandler nate-chandler merged commit 4051ea2 into swiftlang:release/6.2 Jun 6, 2025
5 checks passed
@nate-chandler nate-chandler deleted the cherrypick/release/6.2/rdar152580661 branch June 6, 2025 22:54
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