Instead of building the definition of a thunk immediately when we
generate the thunk declaration, wait until we reach the `}` of the
outermost class, interface, etc. -- at the same time when we would parse
the definition of the thunk if it were defined inline.
This fixes issues where we fail to define the thunk because it requires
an enclosing class to be complete, or its definition depends on
something declared later in the enclosing class.
Make the representation of a suspended function scope, and its
constituent suspended components, be move-only, and switch to passing it
around by rvalue reference instead of by value because it's expensive
both to move and especially to copy.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Right now, return_scope_stack is being used to determine whether logic
is in a function scope. However, we need to handle nested entities
inside function scopes. For example where this crashes right now:
```
base class C(B:! bool) {}
fn F() {
class B {
extend base: C(true or false);
}
}
```
This is doing a few things to make this kind of code not crash:
- Split `scope_stack().Push` into `PushForDeclName`, `PushForEntity`,
`PushForExpr`, and `PushForFunction` so that better decisions can be
made about behaviors.
- Hide `return_scope_stack` in the API, instead using interfaces to get
at the underlying data.
- Also using `PushForFunction` to update it similar to the other stacks
that `ScopeStack` manages.
- Add `IsInFunctionScope` as the best way to determine presence in
function scope.
- Remove `PeekIsLexicalScope` since destruction really wants function
scope information anyways.
- Clean up `destroy_id_stack` handling to be for function scopes rather
than lexical scopes.
- Return after related `context.TODO`s in a couple more spots, so that
code doesn't proceed to add control flow in spite of the lack of
support.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
We frequently want to operate on singletons. Per discussion, drop
`Singleton` to make the code shorter.
This started off as wanting to write `inst_id.is_error()`, but the
dependency relationship between ids.h and singleton_insts.h would
require some kind of delayed evaluation to allow the implementation to
remain in headers (which I suspect is helpful to have for inlining). I
could have added something like `IsErrorInst`, forward declared in ids.h
and defined in singleton_insts.h (which would always be included by
typed_insts.h), but the template approach felt like a decent balance
between (a) removing the boilerplate `::SingletonInstId`, (b)
understandability, (c) still visually mirroring if we immediately return
a singleton, and (d) flexibility for more than just `ErrorInst`. But TBH
I'd probably still have written `is_error()` if it didn't require
addressing the cross-header cycle.
Then I tried `SemIR::InstId::Is<SemIR::ErrorInst>`, which generally
worked with types but generated the complaint that it didn't shorten
*all* singleton uses. So pulling back on `::Is`, and instead just
dropping `Singleton`.
What this does:
- Adds tracking where storage is allocated.
- Determines if that storage supports destruction and, if so, records
the `destroy` function for it.
- Calls any found `destroy` functions when going out-of-scope.
What this does not do:
- Precise scope tracking of temporaries. We currently don't define
temporary scopes, which would probably be the solution.
- Destruction for anything but a `class` with `fn destroy`, in an
implicit return. That excludes:
- Classes with members that need destruction, particularly in the
absence of `fn destroy`.
- Structs, tuples, and arrays.
- Explicit returns, break, continue, nested scopes.
Noting the exclusions in particular, I think those will need work to
support, but this should set the right framework.
The cleanup block concept stems from clang and trying to share code
across cleanups, from discussion with chandlerc. Note in this
implementation I try to find `destroy` functions early on: that's so
that, when destruction is present on multiple paths, particularly
non-shared paths, we only bind the `destroy` method once.
Implementation-wise, I'll note this adds a `has_cleanup` flag to
`TemporaryStorage` and `VarStorage`. There are several related options,
but this felt similar to other information we're trying to track on
instructions. My goal with this is to mitigate the chance of accidental
calls where the storage may not be tracked for destruction. Alternatives
I considered were to not add the flag (I was worried about heightened
risk of errors), or to just add a concept for the relevant `requires`
(which just felt inconsistent).
Cleanup logic ends up in control_flow in this change because I thought
it was a reasonably consistent place for the cleanup block concept and
its pretty direct control flow interactions.
- Explicitly document that `*Param` and `*ParamPattern` insts represent
`Call` parameters.
- Stop wrapping compile-time parameter patterns in `ValueParamPattern`
insts (because they aren't `Call` parameters).
- Document how `MatchContext::results_` relates to the `Call`
parameters, and be more consistent about when it's written to.
- Remove `RuntimeParamIndex::Unknown`: we no longer need to distinguish
"this `Param`'s runtime index is unknown" from "this `Param` isn't a
runtime param", because we no longer use `Param`s at all in the latter
case.
- Rename `RuntimeParamIndex` to `CallParamIndex`.
As a side effect of removing the `ValueParamPattern` insts, this fixes a
minor diagnostic bug where `NoteInitializingParam` didn't identify the
specific parameter that led to a deduction failure, because it expects
generic parameters to only be represented by `SymbolicBindingPattern`s,
but before this change they could be wrapped in `ValueParamPattern`s.
Right now, some post-run logic is does in `Run()`
(`CheckRequiredDefinitions();` and
`context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error());`)
whereas other parts are done by `Finalize`. Noting the goal to move
things off `Context`, this consolidates into a new `FinishRun`. Note
#4962 is adding another bit of post-run that can be consolidated in;
this seems likely to keep growing slowly.
Note this also creates more parity with mutation source, like the
`context_.scope_stack().Pop();` matches the push done by
`CheckUnit::ImportCurrentPackage` and
`context_.inst_block_stack().Pop()` was pushed in `CheckUnit::Run()`.
Also makes `exports()` more consistent with other Context APIs. Makes
`VerifyOnFinish` `const` so that it can't accidentally mutate state, and
is instead only validating that the Context is in its expected
configuration at completion.
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
When declaring a class (or interface), we create a scope that covers the
entire class declaration. If the class was declared in a lexical scope,
we would declare the class name in the innermost scope, which was the
class's own scope instead of the enclosing lexical scope.
Fix this by instead adding the name to the lexical scope at the start of
the class declaration, not the lexical scope created to hold the class.
For now, we reject if the class name would have been shadowed by a name
that has already been declared within its scope, such as a generic
parameter, so we only ever need to modify the end of the list of lexical
lookup results for the class name.
This appears to be sufficient to make local declarations and definitions
of classes and interfaces work properly throughout check, though testing
is pretty minimal so far.
This is essentially the result of looking at `.begin()` uses. We also
frequently do `std::shuffle`, but unfortunately STLExtras doesn't
provide a wrapper for that.
This is for more clearly distinct names, and to make it a clearer
transition from `BuiltinInst` for name conflicts. `FloatType` is also an
instruction, and we have `Carbon::Error` (common/error.h). This avoids
affecting tests, although the name is embedded in the builtin test.
In `LegacyFloatType`, `Legacy` because I was having trouble coming up
with a more appropriate name. I'm not clear this is a `FloatLiteralType`
at present, it needs some work to mirror `IntLiteralType`.
In `ErrorInst`, the suffix `Inst` was discussed as good and similar to
`BuiltinInst` (although I'm trying to get rid of that).
This is resolving a fuzz-discovered crash related to function suspends
and compile time bind indices. Although the crash originally came from
clearly invalid syntax (missing the `=` inside a `class` decl), the
syntax with a value should also be valid but has the same crash.
This approach disallows compile-time bindings in contexts that can
create ambiguous results, particularly class declarations. These are an
issue because a suspended function can have let declarations after it.
I'm allowing them in function bodies and interface scopes.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This switches `DCHECK` and `FATAL` as well.
The goal is to reduce the code size impact of these assertions so that
we can keep more of them enabled. Currently, the largest cost I see from
`CHECK` is not the actual check or the cold code itself, but actually
the failure to inline trivial functions due to the presence of the cold
code. This means that our goal isn't to reduce apparent code size in the
final binary but the LLVM IR cost assessed for these routines in the
inliner, which closely correlates with code size but is a bit different.
As discussed in #4283, experimentation shows that a single function call
with a minimal number of arguments is the lowest cost model for these.
This is easily achieved with a format-string API that internally uses
`llvm::formatv`. This PR is essentially the `CHECK` version of #4283.
However, the check macros are substantially harder to make work with
both format strings and streaming because they also take a condition.
Also, unexpectedly, I was very successful at devising a regular
expression based automated rewrite from the streaming to the format
string form with only low 10s of manual fixes. This includes compacting
strings broken up across lines, etc. Given how well that went, I've
prepared this PR which just directly switches to the format string API
and migrate everything to use it.
One nice side-effect is that the format string approach ends up greatly
simplifying the implementation here as well.
This is ... *shockingly* effective. Parsing speeds up by more than 3%
with just this change. And checking speeds up by **8%** with this change
alone:
```
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 86.3µs ± 1% 82.9µs ± 1% -3.94% (p=0.000 n=17+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 431µs ± 1% 415µs ± 1% -3.76% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 1.77ms ± 1% 1.71ms ± 1% -3.18% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 7.44ms ± 1% 7.17ms ± 2% -3.56% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 30.7ms ± 1% 29.7ms ± 1% -3.15% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 131ms ± 1% 127ms ± 1% -2.81% (p=0.000 n=18+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/256 878µs ± 2% 800µs ± 1% -8.91% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/1024 1.88ms ± 2% 1.72ms ± 1% -8.56% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/4096 5.78ms ± 2% 5.28ms ± 1% -8.70% (p=0.000 n=20+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/16384 21.9ms ± 1% 20.1ms ± 1% -8.02% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/65536 90.4ms ± 2% 83.1ms ± 1% -8.04% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/262144 381ms ± 2% 352ms ± 1% -7.79% (p=0.000 n=19+19)
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
As discussed in toolchain meeting, we want to avoid overloading the
meaning of "instance", and "specific" was the best name we found. It's a
little unorthodox and inventive, but hopefully over time will become as
unsurprising as the term "generic" is.
We can't use the instruction from the generic directly, because it
doesn't have the right constant value. Instead add an instruction that
models the transition from the constant value in the generic to the
constant value in the generic instance.
Also start associating the self generic instance with unqualified
lookups that find results in an enclosing generic, so that we track the
information necessary to create the new instruction.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Based on discussion around the region handling in generic_region_stack,
create a generic structure for the stack-of-vectors support. I also want
to add this to InstBlockStack, but that's a little more complex due to
GlobalInit, so cutting a PR here to check with review.
My work here is how I noticed #4099; I want to be sure that I'm correct
about the issue, but it's the difference between being able to use
PeekArray or not.
Note in scope_stack.h, I believe we could remove next_compile_time_index
and make it just based on elements_size(). However, I want to verify
with you before I make further changes there.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This works to leverage the capabilities of the hashtable as much as
possible, for example using the key context in the value stores.
However, there may still be opportunities to refactor more deeply and
use the functionality even better. Hopefully this is at least
a reasonable start and gets us a clean baseline.
On an Arm M1, this is a 15% improvement on my large lexing stress test,
but ends up a wash on my x86-64 server. This is a smaller benefit than
I expected, and it's because we're using a set-of-IDs and looking up
values with a key context for things like identifiers. This pattern has
a surprising tradeoff. The new hashtable uses significantly less memory,
a 10% peak RSS reduction just from the hashtable change. But indirecting
through the vector of values makes growing the hashtable dramatically
less cache-friendly: it causes growth to randomly access every key when
rehashing. On x86, everything gained by the faster hashtable is lost in
even slower growth. And even on Arm, this eats into the benefits.
But I have a plan to tweak how identifiers specifically work to avoid
most of the growth, and so I suspect this is the right tradeoff on the
whole. It gives us significant working set size reduction and we can
likely avoid the regressed operation (growth with rehash) in most cases
by clever reserving and if necessary by adding a hash caching layer to
the table infrastructure.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I found this through inspection, looking at an array stack data type.
Tests pass either way, not sure what a good test would be for
regressions (tests do fail if the size doesn't match, but either
approach gets an appropriate size). But this is followed by
`truncate(remaining_compile_time_bindings)`, so it seems like
`drop_back` is a better match than `drop_front`.
Build a `Generic` object for generic functions. This object tracks the
generic parameters that are in scope for the generic entity. Eventually
it will track other information about the generic too.
Add basic SemIR formatting support for generic functions.
Following up on discussion from #3948, doing a general rename of
"enclosing scope" to "parent scope" (and "enclosing scopes" to "ancestor
scopes"). The intent is to improve understandability and collide less
with C++ terminology for "enclosing scope". Note this changes most uses
of "enclosing", but leaves behind a few like "enclosing function" and
"enclosing block".
Note this does create some "parent class" mentions for "adapt" and "var"
(the class they're within), which is maybe unfortunate, but we'd
probably say "base class" if we meant inheritance so perhaps that's
okay. Along the same lines, these are the only `parent_class` uses I see
now, and we do have a few `base_class`.
Use a level comparison during substitution to determine whether we're
substituting a particular binding. Evaluate symbolic bindings with the
same name and the same level to the same symbolic constant, for example
across redeclarations of a generic function.
In parse, form a list of methods that are defined inline, tracking where
they start, where they end, and which other inline methods are nested
within them.
In check, when we reach an inline method body, skip it and add it to a
worklist to be processed later. We also track when we reach the start
and end of a context in which inline method bodies are deferred, so that
we know when to replay the bodies.
When suspending a function definition to be processed later, the
`DeclNameStack` entry is moved to separate storage, including popping
the corresponding scopes from the scope stack and removing the
corresponding lexical names from lexical lookup. Later, when we return
to the function and parse its definition, the `DeclNameStack` entry is
restored. The same is done when we reach the end of a nested context
that can have inline methods, so that we can reenter the nested scope
before processing its members.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Richard SmithandJon Ross-Perkins
Thomas Köppe
Boaz Brickner
