incremental: fixes #22403
Merged
incremental: fixes #22403
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
…e changes `Zcu.PerThead.ensureTypeUpToDate` is set up in such a way that it only returns the updated type the first time it is called. In general, that's okay; however, the exception is that we want the function to continue returning `error.AnalysisFail` when the type has been lost, or its number of captures changed. Therefore, the check for this case now happens before the up-to-date success return. For simplicity, the number of captures is now handled by intentionally losing the instruction in `Zcu.mapOldZirToNew`, since there is nothing to gain from tracking a type when old instances of it can never be reused.
mlugg
force-pushed
the
incremental
branch
2 times, most recently
from
97d3ec5 to
db3226a
Compare
This commit reworks how values like the panic handler function are
memoized during a compiler invocation. Previously, the value was
resolved by whichever analysis requested it first, and cached on `Zcu`.
This is problematic for incremental compilation, as after the initial
resolution, no dependencies are marked by users of this memoized state.
This is arguably acceptable for `std.builtin`, but it's definitely not
acceptable for the panic handler/messages, because those can be set by
the user (`std.builtin.Panic` checks `@import("root").Panic`).
So, here we introduce a new kind of `AnalUnit`, called `memoized_state`.
There are 3 such units:
* `.{ .memoized_state = .va_list }` resolves the type `std.builtin.VaList`
* `.{ .memoized_state = .panic }` resolves `std.Panic`
* `.{ .memoized_state = .main }` resolves everything else we want
These units essentially "bundle" the resolution of their corresponding
declarations, storing the results into fields on `Zcu`. This way, when,
for instance, a function wants to call the panic handler, it simply runs
`ensureMemoizedStateResolved`, registering one dependency, and pulls the
values from the `Zcu`. This "bundling" minimizes dependency edges. The 3
units are separated to allow them to act independently: for instance,
the panic handler can use `std.builtin.Type` without triggering a
dependency loop.
These cover the fixes from the last few commits.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Some minor bugfixes, and one very big fix: changing the panic handler no longer makes the compiler explode!
To do this, some new
AnalUnits are introduced which "wrap" the analysis of multiple things in e.g.std.builtin. That way, dependencies on things like the panic handler are represented with a single edge, avoiding dependency bloat and allowing memoization to continue to work.The improved memoization here actually slightly improves the performance of semantic analysis even without
-fincremental:Analyze
stdTestsAnalyze Compiler