Instead, eagerly import only the impls from the api file corresponding
to the current file, if any, because we need those for impl
redeclaration lookup. For all other cases, load only the impls in
libraries that are referenced as part of an impl lookup query.
Building on #4411, avoid using StringLiteral in format strings. This
includes a diagnostic check to prevent regressions (which is also how I
gathered issues).
Note, I haven't looked at `std::string` uses yet, but we might need
things like that to be able to pass strings in code back to the user.
StringLiteral though means that it's literally written down in the
toolchain, at which point it should probably be written in the format
string instead of separately.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
I'm taking the approach of making DiagnosticBase an API so that we can
pass similar diagnostics as parameters. An alternative would be to do
the function_ref approach we've done elsewhere, but these felt more
boilerplate to me.
Note I'm also modifying messages here. Let me know if you'd like
different changes and/or just keeping current formatting (keeping
current formatting would also allow removing some of the templating I've
added, but it felt helpful putting explicit tokens where possible). But
also, things like "`protected` not allowed on `interface` declaration at
file scope" were part of the phrasing issue, I think.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Add a new `specific_function` instruction that represents a generic
function plus its deduced argument list as a callee in a function call.
The new instruction can only appear as the immediate operand of a call
instruction, so we give it a builtin placeholder type.
At the end of each file, require definitions for all specific functions
used in that file. Resolve the generic with the argument list to produce
those specific function definitions as needed, and diagnose if the
generic doesn't have a definition available.
A few tests are updated in cases where they declared and used generic
functions but didn't previously provide a function definition.
This is a primarily automated change:
- Search & replace for capitalization
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s")([A-Z])`
- `$1\L$2`
- Search & replace for period
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s"(?:[^)]|\n)+)\.("[,)])`
- `$1$2`
- Limited search & replace for `ERROR: ` -> `error: ` in streamed things
- Leaving a TODO for command_line because there's more cleanup that can
be done there
- Modify diagnostic_consumer.cpp
- ERROR -> error
- WARNING -> warning
---------
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>
Remove `ReusingLoc` and add enforcement that even for imported
locations, the kind of the parse node for an instruction matches the
kind specified in the instruction definition.
Change the node kind for a few instructions to `NodeId`:
- A couple of instructions had a typed node but could be created
implicitly with any node as part of a builtin implicit conversion. This
happened for `AddrOf`, `ArrayIndex`, and `Deref`.
- A bunch of instructions had `InvalidNodeId` as their associated parse
node kind but were actually always created with a location.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Refactors a bunch of the SemIRDiagnosticConverter to be able to use that
from Lower to access source locations there to use in debug info.
I assume some of this is a bit jank/would need to be fixed/improved in
the future - like the context functor that's passed into ConvertLoc?
(not totally clear what that's for/what the debug info will be missing
out on in its absence, I could throw a FIXME in there if you like)
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I'm trying to make the line of code more clearly nested in crash output
(the way it is, I sometimes forget about it). Also,
`Check::HandleFunctionDecl` is the old naming scheme, it's now all
`Check::HandleParseNode`, so I'm replacing that.
Before:
```
3. extern_library_owner.carbon:6:1: Check::HandleFunctionDecl
extern fn F();
^~~~~~~~~~~~~~
#0 0x0000564c0057ef1d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) ...
```
After:
```
3. extern_library_owner.carbon:6:1: checking FunctionDecl
extern fn F();
^~~~~~~~~~~~~~
#0 0x00005629029ffd9d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) ...
```
The TODO for switching to ReusingLoc (previously Untyped) had been there
for a while, so I'm trying to address it here. The intent had been to be
clearer about when the construction is validated, particularly so that
we aren't accidentally accepting an incorrect NodeId. Note, this does
fix an incorrect use of InvalidNodeId where NoLoc should've been called.
Since this changes the semantics of when `Parse::NodeId` is helpful in
`typed_nodes.h`, I'm doing a pass to either refine or switch to
`Parse::InvalidNodeId` where it compiles. I think most remaining
`Parse::NodeId` examples are things we _should_ be able to refine with a
little more work (versus before where `Parse::NodeId` also indicated
`LocId` construction might be used).
I'm also changing context.h to use `requires` that match what
`LocIdAndInst` has, I think it makes the diagnostics a little better.
And note I do add an overload for `ImportIRInstId`, also matching
`LocIdAndInst`, and widely used for import refs.
Move subtree sizes over to TreeAndSubtrees, using the different
structure to represent the additional parse work that occurs, as well as
making it clear which functions require the extra information. My intent
is to make it hard to use this by accident.
The subtree size is still tracked during Parse::Tree construction. I
think a lot of that can be cleaned up, although we use it during
placeholder assignment so it may take some work. I wanted to see what
people thought about this before taking action on such a change.
I'm using a 1m line source file generated by #4124 for testing. Command
is `time bazel-bin/toolchain/install/prefix_root/bin/carbon compile
--phase=check --dump-mem-usage ~/tmp/data.carbon`
At head, what I'm seeing is:
```
...
parse_tree_.node_impls_:
used_bytes: 61516116
reserved_bytes: 61516116
...
Total:
used_bytes: 447814230
reserved_bytes: 551663894
...
1.43s user 0.14s system 99% cpu 1.565 total
```
With `Tree::Verify` disabled completely, it looks like:
```
parse_tree_.node_impls_:
used_bytes: 41010744
reserved_bytes: 41010744
...
Total:
used_bytes: 427308858
reserved_bytes: 531158522
...
1.20s user 0.13s system 99% cpu 1.332 total
```
Re-enabling just the basic verification (what is now `Tree::Verify`),
I'm seeing maybe 0.05s slower, but that's within noise for my system. I
do see variability in my timing results, and overall I think this is a
0.2s +/- 0.1s improvement versus the earlier (always testing `Extract`
code) implementation. That's opt; debug builds will be unaffected,
because the same checking occurs as before.
Note, the subtree size is a third of the node representation, which is
why I'm showing the decrease in memory usage here.
Allow directly constructing a PostorderIterator, to get rid of
`tree.postorder(node_id).end()` indirect construction. For ranges that
don't need tree data, make it clearer that they're not validated.
Note, this subtly gets rid of a subtree size use in the
`tree.postorder(node_id).end()` case (to get the discarded `begin()`
value).
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.
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>
Also adds import_ir_scope to namespace formatting. I'd done this as an
aid for #4153, and am splitting it out.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This is for consistency with #4120. Similar to that, we can use
overloads on the typed NodeId rather than individually named handlers.
There isn't the same caller benefit here though, since the calls from
check.cpp are already boilerplate.
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>
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
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.
By adding an `ImportDecl` instruction, this creates something that can
be referenced through `ImportIRInst`.
packages/no_prelude/implicit_imports_entities.carbon is getting a test
of this (import_conflict and import_conflict_reverse).
Also re-packs ImportIR from 24 bytes to 16 on 64-bit, since I'm touching
everywhere that makes one anyways.
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>
This refactors how the implicit import is handled in order to retain
more name scope information. As a consequence, private access control
works better between api files and implementation files. Note though
that this will also be essential for name poisoning between the API and
implementation, as discussed in #3763.
In implementing this, I ran into a couple issues with namespaces that I
think point to flaws in their handling. I've fixed some and added a TODO
for the biggest issue (in check.cpp line 281-288), which relates to the
handling of namespaces of direct imports which are first evaluated
indirectly.
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`.
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>
Change syntax for package declaration to put the `impl` keyword at the
start and remove the `api` keyword.
To support this, rearrange processing of package, library, and import
declarations to use the general modifier handling support in declaration
parsing rather than special-case logic.
There is an ambiguity in `impl package.Foo as Bar`, which we resolve by
treating `package` as an introducer after a modifier only if it's not
followed by `.`.
The toolchain was iterating a map of package name to import list in a
couple of places to do things beyond counting or other order-invariant
operations. The result was that the specific hashtable iteration order
influenced the order of SemIR generated (and other behaviors like
diagnostic emission I suspect, but the source location sorting probably
hid this). Switching to a hashtable implementation with any order
seeding that changes run-to-run immediately shows the SemIR order
fluctuating without this.
This PR fixes that by instead accumulating the package imports data in a
vector and using a map to vector indices. This is also slightly more
efficient, although that seems unlikely to be an important factor here.
There was only one insertion point so I've just hand coded the
management of the indices and map, but happy to take a different
approach or use an abstraction here if desired.
One awkward aspect of this is that some of the loops need access to the
package name as well. I've just added storage for that as these seem
unlikely to be huge arrays of 10s of 1000s of imported packages, so the
double storage of the identifier ID seems likely OK. But again, happy to
take a different approach here if desired. It also seems like it might
be possible to work out the identifier from the node, but I kept the
patch more direct for simplicity.
I've also not used the LLVM `MapVector` abstraction of this pattern.
This was mostly to avoid adding another layer of abstractions to our
data structures, and because this is the first time we've hit this
really. My experience is also that it is reasonably often that there is
a more efficient way to orient the vector and map than what is
automatically provided. But that may just be my experience.
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).
I think the new name is more consistent for how `enclosing_scope_id` is
used relative to `name_id` (even removing the clarifying note on
`enclosing_scope_id_for_new_inst`). Suggesting `initial_scope_index` as
a replacing for the old `enclosing_scope`, hoping it's a little clearer.
I'm replacing `target_scope_id` uses in modifier logic because they
seemed to be based on the NameContext use.
This provides `export import` logic in lex, parse, and check; `export
name` logic is only in lex and parse, not check.
I think with `export name` I'm going to need to modify import_ref and
some consolidation logic, whereas `export import` seems feasible to keep
as primarily import logic. Given the implementations were looking like
they'd diverge more substantially, I thought it'd be helpful to cut the
PR here.
When we crash, include the source location of the parse node that we
were handling, as well as the name of the check function that we were
calling. Also include the source snippet, since it's easy to do so, and
may avoid the need to look at the input file in some cases.
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.
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>
This is mainly to prevent a crash. We should typically have a valid
location, but in cases of bugs I think this is better than a simple
check-failure (namely, provides the diagnostic to give better context of
why the diagnostic location looks bad). I also think we might have more
invalid locations in the future.
Note I ran into this because we weren't setting the definition_id for a
class in import_ref; I think this made it much easier to examine.
While most of the examples in our tests are made clear by the code
snippet, sometimes that's not the case. This makes it more clear when
the implicit `Main` package is used for example. I added this because I
got an error message without any source code to show in the snippet
(importing the prelude for example) and this makes the error much more
understandable.
- 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?
This allows impl lookup to find such impls.
Eventually we'll want to do this more lazily, and filter to the relevant
subset of `impl`s needed for a query. But for simplicity, for now just
import all `impl`s.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Adds ImportIRId::ApiForImpl to reserve a specific slot for the `api`
import, so that the code can trivially determine whether an import is
from the same library. This is then used for merging function
declarations, because the rules for redeclarations in the same library
slightly differ as compared to other imports (note they're also not
identical to same-file rules).
The main thing this leaves from the recent #3762 is verifying that
entities forward declared in the `impl` file are also defined, but
that's not in-scope for merging; it's moreso post-checking validation.
Note, a lot of our `invalid <entity> ID` comments in ids.h were
incorrectly copy-pasted, so I've cut `<entity>`.
Instead of attempting to reconstruct this based on the properties of the
target scope of the declaration name, check whether the enclosing scope
of the inner deferred definition scope is the enclosing deferred
definition scope.
Also add verbose logging to the deferred definition handling code to
make it easier to see when we switch between handling the regular parse
nodes and the deferred function bodies.
This doesn't significantly change logic, although I'm trying to add the
location to used state.
The issue I'm trying to address is how to identify a declaration as
"allowed to be redeclared". Consider:
```
library "a" api;
extern fn F();
```
```
library "b" api;
extern fn F();
```
```
library "c" api;
import library "a";
import library "b";
var x: auto = F();
fn F();
```
What currently happens is:
1. On import of "a", `F` becomes ImportRefUnused
2. On import of "b", `F` becomes ImportRefUsed in order to merge.
3. In "c", the call `F()` doesn't change the state.
4. In "c", the declaration `fn F();` needs some breadcrumb to understand
whether "F" has been referenced, as in step (3) here.
What I want to happen is:
1. On import of "a", `F` becomes ImportRefUnloaded
2. On import of "b", `F` becomes ImportRefLoaded in order to merge.
3. In "c", the call `F()` causes `F` to become ImportRefUsed
4. In "c", the declaration `fn F();` detects that `F` is already
ImportRefUsed, and can use the associated `used_id` for a diagnostic
about why redeclaring is invalid.
Note this PR isn't implementing (4). I'm focused on the refactoring to
add a new ImportRef state here.
Richard Smith
Jon Ross-PerkinsandGeoff Romer
David BlaikieandJon Ross-Perkins