Fix a bunch of problems with SILGen of tuples with pack expansions #64231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clients expect a %swift.opaque**; without the cast, local GEPs will go off into the weeds.
and lowered pack types. The "approximate" thing is kindof a representational/testing wart. Really, all that we care about is that we have a formal pack type with the right shape. (We don't *really* need a formal pack type --- we could use anything that can represent the shape --- but we seem to be standardizing on `PackType` for that.) We could just use the reduced shape for that, but for some reason I've been resisting that. Not sure I have a compelling reason, though, and if we decide to use reduced shapes, we can eliminate the approximate formal packs stuff. Inducing packs from tuple slices is a more permanently-useful thing.
Arguably, it would be more consistent with the current design to figure out a representation for pack-expansion components so that we can keep them around in whatever storage. But honestly, I don't like the current design; I think the eager explosion is an over-complicated mistake. So this is partly just me finding an excuse to not extend that to more cases.
More missing infrastructure. In this case, it's really *existing* missing infrastructure, though; we should have been imploding tuples this way all along, given that we're doing it in the first place. I don't like that we're doing all these extra tuple copies. I'm not sure yet if they're just coming out of SILGen and eliminated immediately after in practice; maybe so. Still, it should be obvious that they're unnecessary.
|
@swift-ci Please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two main bugs being fixed in this PR:
The first is that the
RValueabstraction in SILGen was trying to eagerly expand these tuples, which really doesn't work — not only are the instructions it's using not really appropriate for these tuples, there's also just no simple value representation of a pack expansion that can stand in here. Maybe in the long term we'll have value representations, but in the short term, the only reasonable choice seems to be to prevent this expansion. I don't hate that anyway; I've never been a fan of the eager expansion.The second is that the way we were binding parameters didn't work for tuples that contain pack expansions. The current CC recursively expands these tuples and passes expansions as value packs, and the parameter-binding code has to glue everything back together as a tuple. That gluing-together did not know about pack expansions and the different code patterns that have to be used with them. So there was quite a bit of infrastructure that needed to be fleshed out here.