This implements a few closely related features:
- Starts merging namespaces discovered inside imports.
- Stores results of cross-package name lookup as an entry inside the
scope.
- Note this is particularly visible with `i32`.
- Moves more of the imported instructions to the import scope.
Note this is primarily for executing the namespace TODO in check.cpp,
which is removed here.
`testdata/namespace/merging_with_indirections.carbon` tests key
behavior.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This better follows the principle that types are simply constants of
type `type`, and allows more uniform treatment of types as just another
kind of constant from generics handling.
Use a hash table to map from `TypeId` to information about the complete
type. This makes basic operations on types a bit simpler, and operations
that actually need to access the complete class information a bit more
complex.
Changes crash messages to start printing verbose forms of instructions,
rather than just the ID. Fixes some indentation issues with stacks. Also
switches unexpected inst formatting, because now there are lots, and
it'd be helpful to know where they are.
This uses a pimpl pattern for Formatter due to the number of member
functions on Formatter. Maybe we should refactor that, but this didn't
feel like a good place to do so.
Note, I have two concerns about this change... to note them here, to
make sure others are considering them when evaluating the
implementation:
1. Some instructions are very verbose to print, as evidenced by the
fn_decl printing (which includes function params) or scope printing
(which includes scope members).
- I'm not sure whether there's a way to simply reduce this, as it seems
essential to the requested printing of instructions.
- Long-term, we may at least want to limit the number of lines printed
here. However, I've already spent a fair amount of time here and I think
it's in a good state to evaluate.
2. Increased complexity in the crash handler may result in crash
messages failing to generate.
- For example, a crash in Formatter (and its deps, such as InstNamer or
location handling) prevents a stack from being printed. I'm pretty sure
I've written crashes in Formatter before.
Here's an example crash snippet (generated by adding a crash inside
`return` handling) before:
```
2. NodeStack:
0. FunctionDefinitionStart -> function2
1. ReturnStatementStart -> no value
2. IntLiteral -> inst+26
inst_block_stack_:
0. block<invalid> {inst+0, inst+1, inst+2, inst+23}
1. block9 {inst+26}
param_and_arg_refs_stack:
args_type_info_stack_:
```
And after:
```
2. Check::Context
NodeStack:
0. FunctionDefinitionStart: function2
1. ReturnStatementStart: no value
2. IntLiteral:
unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
inst_block_stack_:
0. block<invalid> {
package: <namespace> = namespace [template] {
.Core = unexpected.inst+2
.F = unexpected.inst+23.loc11_22
}
unexpected.inst+1 = import Core
unexpected.inst+2: <namespace> = namespace unexpected.inst+1, [template] {}
unexpected.inst+23.loc11_22: %F.type = fn_decl @F [template = constants.%F] {
unexpected.inst+9.loc11_9: init type = call constants.%Bool() [template = bool]
unexpected.inst+10.loc11_9: type = value_of_initializer unexpected.inst+9.loc11_9 [template = bool]
unexpected.inst+11.loc11_9: type = converted unexpected.inst+9.loc11_9, unexpected.inst+10.loc11_9 [template = bool]
unexpected.inst+12.loc11_6: bool = param b
@F.%b: bool = bind_name b, unexpected.inst+12.loc11_6
unexpected.inst+19.loc11_18: init type = call constants.%Int32() [template = i32]
unexpected.inst+20.loc11_18: type = value_of_initializer unexpected.inst+19.loc11_18 [template = i32]
unexpected.inst+21.loc11_18: type = converted unexpected.inst+19.loc11_18, unexpected.inst+20.loc11_18 [template = i32]
@F.%return: ref i32 = var <return slot>
}
}
1. block9 {
unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
}
param_and_arg_refs_stack:
args_type_info_stack_:
```
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>
This adds `DefinitionInfo` for `Define`-based configuration so that
parameters are optional. It also makes it easier to provide the
equivalent functions on both `Definition` and `Define`.
A common pattern used here is to change from a `switch` with in-line
`case`s to instead have `case`s that call an overloaded function. What's
happening here is that the instruction type is used to select an
overload, and if an overload is not defined, a compiler error would
result. Meanwhile, clusters of overloads are being defined using
`requires`-based templating, so that equivalent implementations are not
copied. This addresses a limitation of a vanilla `switch` approach where
it's hard to have redundant cases using conditional logic, while also
getting compiler errors when adding new `InstKind` entries, which had
been a significant part of why we used macros previously.
This starts hitting some odd clang-format edge cases causing
`CARBON_KIND_SWITCH(inst){` (missing space), which I haven't seen
before. Adding `CARBON_KIND_SWITCH` to .clang-format works around it.
I'm trying to increase the distinction between BuiltinKind and
BuiltinFunctionKind. BuiltinKind is for instructions,
BuiltinFunctionKind is for function definitions. To get to this point,
I'm doing a few changes:
- BuiltinKind -> BuiltinInstKind
- builtin_kind.* -> builtin_inst_kind.*: filename consistency
- Builtin -> BuiltinInst: mainly for consistency with the above
- Builtin::builtin_kind -> BuiltinInst::builtin_inst_kind: somewhat
repetitive but seems like a consistent edit
- Function::builtin_kind -> Function::builtin_function_kind: seems a
useful distinction
I'm leaving alone things like (and mentioning in case there's a desire
for more renames):
- InstId::BuiltinError, InstId::ForBuiltin: these I think are more
apparent because they're directly associated with Inst.
- GetBuiltinICmpPredicate in lowering: maybe builtin function handling
should be in its own file, but these local names don't feel problematic
to me.
- GetBuiltinType, BuildBuiltinValueRepr, PerformBuiltinIntComparison:
similar to the above, names don't feel too problematic
Require types into which qualified lookup is performed to be completely
defined. Eventually this will trigger substitution into the definition
for generic types.
This executes on a TODO in AddImportRef to add instructions to their own
block instead of the File block. This has an important consequence of
removing a pattern from InstBlockStack that added to blocks not
currently at the top, cleaning up an issue for ArrayStack. The delta
here is then mostly in different formatting of the import refs, a
consequence of the separation.
Creates a `GlobalInit` class for storing relevant values, pulling
functions off `InstBlockStack` and `Context`. Adds a `Context` pointer
just so that it doesn't need to be passed in on each call (`Finalize` in
particular uses several members).
Note we have several different `InstBlockStack` instances, so several
copies of the relevant members were simply unused.
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>
When checking a declaration or definition of a generic, track a list of
created instructions that depend on the generic's parameters in some
way, along with information on how they depend on the parameters. This
will eventually be used to determine what information we need to compute
when creating instances of the generic, but for now we're just building
the list.
Information is tracked separately for the declaration region and the
definition region of the generic, because in general these may be first
provided in separate declarations, and they should be substituted into
at different times.
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.
Name scopes store the names in their scope in a `DenseMap`. Several
places reasonably avoid depending on the iteration order by sorting the
names -- they're in the formatting code path where that's a solid
approach.
Unfortunately, when we're importing one scope into another, we also need
to walk the entire scope and do something for each name. =[ This doesn't
seem like a great place to sort things to stabilize them.
I've switched to a fairly simplistic solution of having a vector of name
entries that can be iterated stably, and a separate map for lookups. I
didn't use the set-of-indices trick here because it's not clear that's
the right trade-off for a scope: likely a lot of small scopes here with
relatively hot name lookups. And the key here isn't a large or
dynamically sized thing that we're canonicalizing, it's a `NameId`. That
made me lean towards duplicating the name in the hashtable for lookup
and the vector for iteration.
I thought about a fancy approach of sorting the hashtable keys by their
values (the indices), but that would still require a bit of copying and
more code.
I also thought a bit about other optimizations, but decided to leave a
comment for now -- it's not obvious to me exactly how hot this is and
whether it's better served by faster lookups, being more memory dense,
etc. And that might involve more of an SOA layout change or some other
approach. Rather than do that here, and especially before switching
hashtables, I stuck with a simple approach to address the ordering.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Require mapping from a `ConstantId` to an `InstId` to go through the
`ConstantValueStore`.
This is a preparatory step for an upcoming generics change where
symbolic `ConstantId`s are no longer just a thin wrapper around an
`InstId` but instead are indexes into a table with additional
information about the symbolic constant beyond its `InstId`.
Instead of redundantly storing both the `return_type_id` and
`return_storage_id`, where the declared return type is just the type of
the return storage, store only the `return_storage_id`.
Add a convenience property to get the declared return type of the
function.
In addition to avoiding storing redundant information, this is a
preparatory step for an upcoming change for generics support that will
make it more expensive and awkward to store `TypeId`s in places other
than the type of an instruction.
This is mostly mechanically duplicating work done for generic classes to
also support generic interfaces.
Also fix both generic interfaces and generic classes to support
importing class and interface types with arguments from another file.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Adds access to the name lookup table in name scopes. This is so that we
can quickly check access during name lookup without resolving the entity
itself. Does this for names in general, but does not implement handling
for entity-scoped names, only namespace-scoped names (where they're
essentially just not exported).
Excludes `private` names from exports. Although names should be
accessible to `impl` files, that's not implemented here because we'll
probably want to do it by directly copying name lookup tables.
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`.
Just spotted these while looking at warnings that seem to fire on our
code are probably are things we'd fix if we saw them. None of these seem
important FWIW.
Also removes a redundant flag that is part of `-Wall`.
I have a follow-up for the high-value warning I spotted that motivated
me to look at all of this. But it's noisy so kept it as a separate PR.
Trying to conform with #4009. Changes SemIR::LocIdAndInst construction
to root out struct init cases with AddInst and related functions. I'm
using templating of AddInst functions in order to avoid `AddInst(loc_id,
InstName{...})` and instead have `AddInst<InstName>(loc_id, {...})` with
I think similar readability results. There are a couple cases where inst
construction is templated and so designated initializers couldn't be
used, so this may be better for those in particular due to the extra
type enforcement.
This probably doesn't clean up every last case, but I was trying to get
the bulk at once without bleeding over into less related changes.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Previously we did some of this in decl_name_stack and some of it in the
callers of decl_name_stack. Factor out a single place to pop a name and
its optional following parameters.
Part of making this behavior consistent is that we now track whether an
implicit parameter list was present or absent rather than mapping an
absent list to `InstBlockId::Empty`. This improves our redeclaration
checking and the precision of some diagnostics.
Switch from recursing into non-canonical instruction fields to
separately canonicalizing those fields. This means we now form canonical
`InstBlockId`s, `TypeBlockId`s, `IntId`s, `FloatId`s, and `BindNameId`s
at least in the cases when they're referenced by a constant instruction.
This reduces the overall runtime for @chandlerc's 10MLoC example by
27.5% on my machine.
This adds a `BindExport` instruction in order to better track the
location of the `export` itself, but a `bind_name_id` is also added to
`ImportRef` so that we know quickly where to put it in name lookup.
Merging identical names is a TODO. I haven't quite decided how best to
achieve that, because I do think the BindExport should be what's
actually added to name lookup.
Also, I will probably add a mode to DeclNameStack that blocks
non-namespace scopes. This seems to already be an error, but the wrong
one (maybe due to lack of support for cross-file decl/def support).
This is modeled analogously to FunctionType. So far, GenericClassType
has no operations, but eventually values of that type will be callable
like values of FunctionType.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This was to track use of a declaration after import, prior to a
redeclaration. Per [discussion on
Discord](https://discord.com/channels/655572317891461132/1217182321933815820/1236016521059237962),
we likely don't need this check due to the change in behavior of
`extern`.
Rather than potentially getting one of many `extern` decls and depending
on it by accident, it is now planned to be _required_ to be imported,
and the library doing a non-`extern` decl must _know_ it's importing the
`extern` decl. The stricter requirement on the library means it now
seems more reasonable to use the `extern` decl.
So kind of rolling back #3831, though keeping `ImportIRInstId` (at least
for now) and keeping `Loaded`/`Unloaded` terminology (seems a nicer
fit).
This removes the builtin FunctionType, replacing it with a FunctionType
instruction. The constant for a FunctionDecl is now a StructValue with
type of FunctionType.
Note this means a function declaration produces _both_ a type, and a
value of the type. This has some consequences in terms of circularity,
and makes the importing of function declarations a little more complex.
It'll get particularly peculiar for imports because of the behavior of
the reference, but that's a known issue due to other things such as
`alias`. The impact will hopefully be contained to
ResolvePrevInstForMerge (and ImportRefs).
To note a small formatting change in diagnostics:
```
- // CHECK:STDERR: fail_member_lookup.carbon:[[@LINE+4]]:3: ERROR: Value of type `<associated <function> in Interface>` is not callable.
+ // CHECK:STDERR: fail_member_lookup.carbon:[[@LINE+4]]:3: ERROR: Value of type `<associated F in Interface>` is not callable.
- // CHECK:STDERR: fail_todo_facet_lookup.carbon:[[@LINE+4]]:3: ERROR: Value of type `<associated <function> in Interface>` is not callable.
+ // CHECK:STDERR: fail_todo_facet_lookup.carbon:[[@LINE+4]]:3: ERROR: Value of type `<associated F in Interface>` is not callable.
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
As we talk about how imports should work, we want to start having
indirectly imported IRs in `import_irs`, where it's currently only
directly imported IRs. The tracking of `CheckIRId` is set up to be able
to determine whether an IR being indirectly imported is actually already
tracked (and may be a direct import). Changes here to how ApiForImpl is
handled start using the logic.
Note that indirect imports may be added while processing even the first
import from the current library. As a consequence, it's necessary to
prepare this map before starting imports, in particular, setting
ApiForImpl before any indirect imports may encounter the same import.
Also, prior validation that `num_irs` matches the final size are no
longer relevant.
check_ir_id is tracked on SemIR because I want to be able to figure it
out given an ImportIR, but the mapping is ephemeral after checking and
so I store that in Context.
I'm putting relevant logic into import_ref.cpp because the primary
alternative is import.cpp, and as more logic is added, ImportRefResolver
will be adding import IRs.
Make constant emission non-recursive, and stop building a bogus
FunctionContext to emit constants.
To support this, move `InstConstantKind` from the typed instruction
definition into the `.def` file, and add more macros to allow us to
generate case labels based on whether an instruction is a constant.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This allows ImportRefs to point at a potentially distant instruction,
while still providing a LocId that can be used for diagnostics. I think
this'll be used if we're trying to point ImportRefs at canonical
instructions in distant IRs. It conveniently eliminates a special-case
in check.cpp.
Restructures LocIdAndInst::Untyped because I think it's not really
needed after this change (the key non-import use was GetWithLocId, which
I just give friend access for). Adding NoLoc for things that don't
provide _any_ location because it turns out Inst can easily be passed in
this way which was not what I had intended, but is used.
We don't need it any more, and removing it simplifies a few things:
- One fewer predefined `File` and reserved ID.
- We now have simply `Builtin` instructions for builtins, instead of
having an `ImportRef` that indirectly references a `Builtin`.
- `ConstantId`s now always refer directly to a local constant, instead
of sometimes referring to an `ImportRef` for a constant in the builtins
IR.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Adds support for unary `-` and binary `+`, `-`, `*`, `/` for floating
point types.
Real literals are now transformed to `llvm::APFloat`s during the check
phase into the `FloatLiteral` instruction.
This PR likely collides a bit with #3892 and might need to be updated
when that one is merged.
Allow an explicit `as` conversion to convert between adapters and their
adapted types. Also make the value representation of an adapter be the
same as the value representation of the adapted type so that the
conversion is always possible.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Move completeness check to the point where the function is defined or
first called. This means we also defer deciding whether the function has
a return slot until that point. Instead of storing a return slot per
function, store the location of the return storage, which may or may not
be used, and compute and store a separate flag saying whether to use it
at the point of first use or definition.
This is the final piece in supporting simple `Make` functions in classes
as a replacement for constructors.
`i32` is retained as a special case for now, for bootstrapping purposes,
and maps to `BuiltinIntType`, which is distinct from `Core.Int(32)`.
This will be removed later once we support `Core.BigInt`.
For now this provides both the `iN` types and also the builtins to
support `Core.Int(N)`. The intent is that we'll change the `iN` support
to rewrite to calls here when we do that for the other type literals and
type keywords.
No conversions between integer types are supported yet, and all literals
are of type `i32`, so we can't actually form values of any of these new
types.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
- Adds an empty prelude.carbon file
- Imports that file in any non-Core package file
- Adds --disable-prelude-import to avoid that
- Adds --exclude-dump-file-prefix to be able to hide files from dumping
- Used to hide core files (we can't do this by package name due to lex
dumps, for example)
- Restructures some tests to not rely on `i32`, particularly `alias`
tests (which rely on a name ref) and tests with no prelude.
I'm adding the framework for switching i32 to calling Int32 in the
prelude, but I'm running into a separate error actually switching over.
So that *mostly* works, but isn't quite ready for prime time. However,
maybe the current state of this PR is still useful to review since it
does a lot of the infrastructure work and adds the %Core everywhere?
LookupNameInDecl is only called from DeclNameStack, but I'm adding
mark_imports_used there because it feels more consistent. Not sure if we
want a better API boundary. I admit I'm also suspicious of its call to
LookupInCurrentScope but maybe it's okay due to how imports work.
I was choosing to print multiple diagnostics when a declaration is
previously used _and_ doesn't match because I think the "previously
used" is more important, but the "doesn't match" may give an additional
hint about why it didn't work.
The merge.h utility function is because I think we can follow a similar
model for identifying errors with other declarations: classes,
interfaces, etc.
Jon Ross-PerkinsandRichard Smith
Geoff Romer
Chandler CarruthandJon Ross-Perkins
Pablo Paglilla
