Implement parameter arity reabstraction #64476
Merged
Conversation
This file contains hidden or 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
|
@swift-ci Please test |
rjmccall
force-pushed
the
arity-reabstraction
branch
from
463dca9 to
e3c0f0c
Compare
|
@swift-ci Please test |
rjmccall
force-pushed
the
arity-reabstraction
branch
from
e3c0f0c to
2ef4729
Compare
|
@swift-ci Please test |
We'll be using these results with packs.
…st function parameters This is necessary because the use patterns in SILGenPoly require walking two orig+subst sequences in parallel, which poses problems for a callback-centric design like the one I addded before. An inversion of control is necessary; this is basically a manual coroutine. But frankly it's a nicer interface than the callback design, too; I switched the implementation of forEachFunctionParam to use the generator just to avoid code duplication, but I might try to remove it and switch all the clients over to the generator. The main problems with the callback design are that (1) I wasn't sure which values clients would want, and the result is that there are a *lot* of parameters and (2) (relatedly) the types of those parameters have to be written out explicitly, and some of them are quite large. The generator code is just much nicer. Maybe I can still give the generator a little unparameterized callback to keep lexical loops simple. I'll need to do this same thing for tuples. One at a time.
…eter and not also drop a subst parameter. This turned out to be more convenient for certain clients (e.g. SILGenPoly) than requiring the full subst param list to be passed in. These clients want to process the subst param list separately, and dropping self early can be convenient for that. The only fundamental reason we need this flag is for working with the orig type, so just use it for that; clients that need to use this feature can reasonably be expected to cooperate.
rjmccall
force-pushed
the
arity-reabstraction
branch
from
2ef4729 to
8190924
Compare
|
@swift-ci Please test |
Yeah, this is much cleaner.
rjmccall
force-pushed
the
arity-reabstraction
branch
from
8190924 to
0d7ec6d
Compare
|
@swift-ci Please test |
This is largely a matter of changing the main loop over subst params in TranslateArguments to use the generators I added, then plugging back into the general reabstraction infrastructure. Because we don't have pack coroutines, we're kind of stuck in the code generation for pack reabstraction: we have to write +1 r-values into a temporary tuple and then write those tuple element addresses into the output pack. It's not great. We also have lifetime problems with things like non-escaping closures --- we have that problem outside of reabstraction thunks, too. Other than that glaring problem, I'm feeling relatively good about the code here. It's missing some peepholes, but it should work. But that that's not to say that arity reabstraction works in general; my attempts to test it have been exposing some problems elsewhere, and in particular the closure case crashes, which is really bad. But this gets a few more things working, and this PR is quite large already.
rjmccall
force-pushed
the
arity-reabstraction
branch
from
0d7ec6d to
a05fef5
Compare
|
@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.
This is largely a matter of changing the main loop over subst params in
TranslateArgumentsto use the generators I added, then plugging back into the general reabstraction infrastructure.Because we don't have pack coroutines, we're kind of stuck in the code generation for pack reabstraction: we have to write +1 r-values into a temporary tuple and then write those tuple element addresses into the output pack. It's not great. We also have lifetime problems with things like non-escaping closures --- we have that problem outside of reabstraction thunks, too.
Other than that glaring problem, I'm feeling relatively good about the code here. It's missing some peepholes, but it should work. But that that's not to say that arity reabstraction works in general; my attempts to test it have been exposing some problems elsewhere, and in particular the closure case crashes, which is really bad. But this gets a few more things working, and this PR is quite large already.