Defer resolving the eval blocks and value blocks of generics and specifics until we've finished other resolution work. #4202
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
specifics until we've finished other resolution work. This avoids import cycles, and reduces the amount of temporary vectors we build before retrying. Import the self specific when importing a generic, now that doing so doesn't introduce cycles.
jonmeow
approved these changes
Aug 8, 2024
| @@ -1811,6 +1798,106 @@ class ImportRefResolver { | |||
| .element_type_id = context_.GetTypeIdForTypeConstant(elem_const_id)}); | |||
| } | |||
|
|
|||
| // Perform any work that we deferred until the end of the main Resolve loop. | |||
| auto PerformPendingWork() -> void { | |||
| while (!pending_generics_.empty() || !pending_specifics_.empty()) { | |||
There was a problem hiding this comment.
This is weird, can you comment it. :)
toolchain/check/import_ref.cpp
Outdated
Comment on lines
1852
to
1864
| auto* local_generic = &context_.generics().Get(pending.local_id); | ||
| local_generic->decl_block_id = decl_block_id; | ||
|
|
||
| auto self_specific_id = MakeSelfSpecific(context_, pending.local_id); | ||
| local_generic->self_specific_id = self_specific_id; | ||
| pending_specifics_.push_back({.import_id = import_generic.self_specific_id, | ||
| .local_id = self_specific_id}); | ||
|
|
||
| auto definition_block_id = | ||
| ResolveLocalEvalBlock(import_generic, pending.local_id, | ||
| SemIR::GenericInstIndex::Region::Definition); | ||
| local_generic = &context_.generics().Get(pending.local_id); | ||
| local_generic->definition_block_id = definition_block_id; |
There was a problem hiding this comment.
The use of a pointer (and reassignment) looked a little odd to me, maybe:
Suggested change
| auto* local_generic = &context_.generics().Get(pending.local_id); | |
| local_generic->decl_block_id = decl_block_id; | |
| auto self_specific_id = MakeSelfSpecific(context_, pending.local_id); | |
| local_generic->self_specific_id = self_specific_id; | |
| pending_specifics_.push_back({.import_id = import_generic.self_specific_id, | |
| .local_id = self_specific_id}); | |
| auto definition_block_id = | |
| ResolveLocalEvalBlock(import_generic, pending.local_id, | |
| SemIR::GenericInstIndex::Region::Definition); | |
| local_generic = &context_.generics().Get(pending.local_id); | |
| local_generic->definition_block_id = definition_block_id; | |
| auto& local_generic = context_.generics().Get(pending.local_id); | |
| local_generic.decl_block_id = decl_block_id; | |
| auto self_specific_id = MakeSelfSpecific(context_, pending.local_id); | |
| local_generic.self_specific_id = self_specific_id; | |
| pending_specifics_.push_back({.import_id = import_generic.self_specific_id, | |
| .local_id = self_specific_id}); | |
| // This may invalidate local_generic. | |
| auto definition_block_id = | |
| ResolveLocalEvalBlock(import_generic, pending.local_id, | |
| SemIR::GenericInstIndex::Region::Definition); | |
| context_.generics().Get(pending.local_id).definition_block_id = definition_block_id; |
There was a problem hiding this comment.
I started with something like this, but it seemed error-prone to me and harder to systematically get right. Changed to a different approach, what do you think?
toolchain/check/import_ref.cpp
Outdated
Comment on lines
1896
to
1897
| local_specific = &context_.specifics().Get(pending.local_id); | ||
| local_specific->definition_block_id = definition_block_id; |
There was a problem hiding this comment.
Similar to the above, I'd suggest only reusing the same variable when it's significant (which for line 1892, it is, but that doesn't look true here).
Suggested change
| local_specific = &context_.specifics().Get(pending.local_id); | |
| local_specific->definition_block_id = definition_block_id; | |
| context_.specifics().Get(pending.local_id).definition_block_id = definition_block_id; |
There was a problem hiding this comment.
Note, suggesting this mainly because I find it easier to figure out what the source is when it's assigned in fewer places.
jonmeow
reviewed
Aug 8, 2024
toolchain/check/import_ref.cpp
Outdated
Comment on lines
1849
to
1853
| // Don't store this between calls: the generics list can be reallocated by | ||
| // ResolveLocalEvalBlock importing more generics. | ||
| auto local_generic = [&]() -> auto& { | ||
| return context_.generics().Get(pending.local_id); | ||
| }; |
There was a problem hiding this comment.
Maybe move onto the type as a function, so calls look like pending.Get(context)? (lambdas always make me hesitant)
There was a problem hiding this comment.
Not, discussed offline, just inlining the fetches
jonmeow
approved these changes
Aug 8, 2024
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 avoids import cycles, and reduces the number of temporary vectors we build (and potentially throw away on retry). Import the self specific when importing a generic, now that there's no risk that will introduce cycles.
Note that we could take the same approach to import classes, interfaces, and so on, instead of the current third phase of resolution for those instructions, but in this PR I'm just addressing the import cycle I'm currently seeing in a work-in-progress PR.