We can lower the same variable from multiple `SemIR::File`s if we reach
it through a specific for a generic in a different file. When this
happens, reuse the same global variable in lowering. This matches how we
handle the same problem for functions.
Assisted-by: Claude via Antigravity
See access_private_parent_member_in_unrelated_class_with_friend.carbon
for an example of this fix.
The problem with that example, prior to this fix, is that when
`Derived::F` is accessed, clang tries to find `F` via
`CarbonExternalASTSource::FindExternalVisibleDeclsByName`, but
`LookupQualifiedName` doesn't find anything because it (correctly) sees
that `F` is a private member of `Base`, so it's not accessible. What it
doesn't know is that it will be accessed from the context of `A`, which
is a friend of `Base`, and so privacy doesn't matter.
`FindExternalVisibleDeclsByName` is really supposed to report visible
items, regardless of whether they can be accessed. But changing it to
simply ignore access (e.g. by modifying `LookupQualifiedName` to
optionally ignore access) doesn't quite work here. This can make a
private member of a base class appear within the `DeclContext` of a
derived class with access set to private, when it should be set to none.
This causes an assert in clang (this case is tested in
fail_access_private_parent_member_in_unrelated_fn.carbon).
Instead, change `FindExternalVisibleDeclsByName` to call
`LookupNameInExactScope` instead of `LookupQualifiedName`. This prevents
extended scopes from being searched, so members of a base class are not
found when filling in the `DeclContext` of a derived class. Clang will
call `FindExternalVisibleDeclsByName` in the context of the base class,
and the base class members are added at that time.
When building an expression referencing the result of name lookup, if
the lookup result is non-constant then check we can access it from the
current scope.
* If it is a global, then use a new `GlobalRef` instruction to get the
value of that global. In lowering, create global variables to hold the
values of global `let` bindings so that `GlobalRef` can access those
values.
* If it is a local, walk the lexical scopes between it and the point of
use and ensure it is usable through from all of them. Error if there's
an intervening declarative scope such as a class, add a TODO to handle
captures for intervening functions, and allow `typeof(...)` to name
things it can't read the value of since it's unevaluated.
Global `let`s (which includes `let`s at class scope) now also have their
initializers collected into the global inits block. If the initializer
is non-constant, the wrapper binding also lives in the global inits
block; otherwise it continues to live in the file block and the variable
emitted for it is a global constant.
Assisted-by: Gemini and Claude via Antigravity
These tests were originally generated by David with Claude as part of
reviewing #8075; see
https://github.com/carbon-language/carbon-lang/pull/8075#issuecomment-6616160559
for more background. I've gone through the tests and made some changes
and cleanups. There are a lot of tests, and sometimes the logic of what
should be accessible can be a little subtle, so putting this up for
review separately before trying to fix any of the tests.
Co-authored-by: David Blaikie <dblaikie@gmail.com>
Historically, we disabled the rendered form in tests outside of
`toolchain/diagnostics`. The idea was to keep tests shorter and avoid
very long chains of autoupdate before stabilizing due to all the line
numbers and other artifacts in the rendered form.
This PR adds `--diagnostic-line-numbers=[all|first]` and defaults to
`first` in tests. This keeps the rendered form so that we can easily
code review that form and the visual cues it does or doesn't provide.
But it removes all but the first location's line numbers to provide
significantly more stability in tests.
By default, tests get the full unicode line-drawing form of diagnostics
so we can ensure all of the artifacts there make sense with the specific
case being diagnosed, but we do retain some focused ASCII-rendering
tests to ensure that fallback remains usable.
There are some issues here that will be fixed in subsequent PRs.
Notably, we try to keep the frame together in the check lines, and so
force some parts to use the first location. This largely works, but
fails in some edge cases, causing the entire frame to be located
incorrectly. There is a TODO about this that we need to fix, and it only
occurs 6 times across the test suite, so it didn't seem worth
front-loading a fix for here.
Assisted-by: Claude Code
When impl lookup results in an `ImplWitness`, either directly or later
through `LookupImplWitness` evaluating to an `ImplWitness` inside some
specific, it represents a dependency on the eval blocks of the
`ImplDecl` that the `ImplWitness` comes from. The `ImplWitness` contains
a `SpecificId` of the `ImplDecl`, and the constants in the witness table
must be viewed through that `SpecificId`. The constants may depend on
the declaration or the definition block of the `ImplDecl`'s specific, so
any use of the witness table from `ImplWitness` requires the specific
from the `ImplWitness` to be fully resolved: both the decl and
definition blocks.
The specific declaration block is always resolved immediately when
forming a specific id, so there's nothing to do there. And things are
simple when the `ImplWitness` returned from impl lookup is concrete. We
would simply resolve the specific definition directly in the evaluation
of `LookupImplWitness`.
But when the `ImplWitness` is symbolic, meaning there's a dependency on
generic parameters in its referenced specific, then we have a dependency
on resolving the specific definition in the `ImplWitness` for every
specific that gets applied to the `ImplWitness`, eventually resulting in
a concrete specific.
Previously we attempted to do this by representing a dependency edge
from the eval block containing the `ImplWitness` instruction to the
definition block of the specific inside the `ImplWitness`. We
represented this dependency edge with the `RequireSpecificDefinition`
instruction.
The challenge with the `RequireSpecificDefinition` instruction was
ensuring it always landed in the _same eval block_ as the one holding
the symbolic `ImplWitness`. Ultimately, we've found a way to do this
without the `RequireSpecificDefinition`. The witness table is accessed
through `ImplWitnessAccess`, so we resolve the specific definition
before each access to the witness table. If the definition is already
resolved, this just costs a lookup of the Specific from the value store.
We document the other things we tried, which don't work or were too
complex, below.
In PR #6236, we added the `RequireSpecificDefinition` instruction in the
eval of `LookupImplWitness`, but this is too late. If impl lookup
resolved to a symbolic `LookupImplWitness`, the `LookupImplWitness` is
added some eval block. Later, we form specifics and re-evaluate that
eval block, and the `LookupImplWitness` can evaluate to an
`ImplWitness`. Adding the `RequireSpecificDefinition` instruction at
this point inserts the instruction into whatever generic's eval block
happens to be on the top of the generic stack, not the eval block of the
generic we are building a specific for. And indeed we can't add
instructions to a generic's eval block later like this, as it would be
missing from specifics that were already resolved for the generic. This
incorrect insertion shows up as a crash in eval at the CHECK introduced
but commented out in [eval.cpp in
#7981](https://github.com/carbon-language/carbon-lang/blob/56d9bbc40add1e9263f2f9b2555c3a27292d1725/toolchain/check/eval.cpp#L3507-L3512).
This is covered by the
`require_specific_impl_definition_through_specific_instantiation.carbon`
test.
We tried moving the insertion of `RequireSpecificDefinition` to be
earlier - at the point where the `LookupImplWitness` is constructed, in
the `LookupImplWitness()` function, instead of inside evaluation. This
seemed like a straightforwardly correct answer, as the eval block that
contains the `LookupImplWitness` will then also contain the
`RequireSpecificDefinition`. However, this can cause us to lose the
`RequireSpecificDefinition` entirely. Because it's a separate
instruction, not an operand of `LookupImplWitness`, it does not get
treated as a dependency of the witness. When we do an impl lookup inside
an `alias`, we build a generic for the alias and then discard it. This
discards the eval block containing the `LookupImplWitness`. Then, in an
enclosing generic, we refer to the constructed `AliasDecl` instruction,
which pulls all its dependent operands into the enclosing eval block.
This pulls in the `LookupImplWitness` into the enclosing eval block, but
it does not preserve the `RequireSpecificDefinition`. This is covered by
the
`require_specific_impl_definition_from_lookup_impl_witness_in_class_definition.carbon`
test.
We tried having `ImplWitness` evaluation resolve the specific definition
of the specific `ImplDecl` it refers to. This seems like the simplest
approach by far as we should always be working with the correct eval
block and nothing can get lost. However it does not work because of the
recursive nature of `ImplWitness`. The `ImplWitness` instruction appears
in the declaration of an `ImplDecl`, and the `ImplWitness` holds a
`SpecificId` for that same `ImplDecl`. When building a specific
`ImplDecl`, we evaluate the `ImplWitness` in the `ImplDecl`'s
declaration block which then immediately resolves the specific
definition before the declaration is finished. This leads to missing
instructions in the specific definition. In short, this puts the
dependency edge to the `ImplDecl`'s specific inside the `ImplDecl`
itself, which is problematic.
We tried wrapping `ImplWitness` in another instruction,
`ImplWitnessUse`. Inside the `ImplDecl` declaration there is still
`ImplWitness`, but then in the definition there is `ImplWitnessUse` and
that is what is returned from the evaluation of `LookupImplWitness`. The
`ImplWitnessUse` is an `ImplWitness` plus it ensures the specific
referred to by the witness has its definition resolved. However this
breaks constant comparisons, as we get different witnesses inside an
impl versus outside.
The first iteration of this PR's solution had us insert the
`RequireSpecificDefinition` into the eval block when we sniffed a
`LookupImplWitness` instruction, however that missed cases where impl
lookup immediately resolved to a symbolic `ImplWitness`, which is why we
now sniff for the `ImplWitness` in the eval block. This is covered by
the
`require_specific_impl_definition_from_symbolic_impl_in_class_definition.carbon`
test.
We also originally made a mistake of not inserting
`RequireSpecificDefinition` when inside the impl definition that the
`ImplWitness` refers to. But we can form specifics ImplWitness values
through function signatures, and these must add the dependency edge on
that specific ImplDecl definition. This is covered by the
`require_specific_impl_definition_from_inside_impl_definition.carbon`
test.
Finally, we did have a working solution. It was to sniff instructions
added to the eval block of a generic, and when an `ImplWitness` is added
to the eval block (it is implied that it is symbolic), also add a
`RequireSpecificDefinition` to that eval block immediately. We also
resolved the definition of a specific referred to by `ImplWitness` when
`LookupImplWitness` evaluates to `ImplWitness` to cover the concrete
witness case. This complicated the construction of the eval block
immensely, however. It caused a situation where adding one inst to the
eval block actually adds two, which we needed to avoid when importing.
And it required inst-kind specific behaviour in the very general
construction of an eval block, completely outside of eval itself.
When a Carbon class from another library was used from C++, its fields
were never exported, so C++ member access failed with 'no member named'.
The class scope of an imported class holds import references rather than
`FieldDecl`s, so `LookupClassFieldByStructField` didn't find them.
Resolve scope entries to their constant `FieldDecl`, load the import
references before exporting fields, and consistently key exported fields
by the `FieldDecl`.
Unfortunately `SemIR::LookupClassFieldByStructField` can't call
`LoadImportRef` because `SemIR::LookupClassFieldByStructField` is in
`SemIR` and `LoadImportRef` needs a `Check::Context` (and `check`
depends on `sem_ir`, not the other way around, so `sem_ir` can't use
check). So instead, make a `Check::LookupClassFieldByStructField` that
wraps the `SemIR` version, and does the `LoadImportRef`
Assisted-By: Claude
---------
Co-authored-by: Christopher Di Bella <cjdb.ns@gmail.com>
A `where` expression can have symbolic references to `.Self` inside it,
but those references do not represent symbolic instructions that should
be part of an enclosing generic eval block. They are internal details of
the facet type, and as such they are PeriodSelfSymbolic, not Symbolic.
The WhereExpr evaluation is careful to produce a Symbolic constant value
only if it has symbolic dependencies on the LHS of the `where`, which
avoids leaking the symbolic nature of `.Self` into he facet type's
constant value.
But until now, those PeriodSelfSymbolic insts could still leak into the
enclosing eval block if they ended up in an operand of a Call
expression. A compile-time Call expr evaluates to a TemplateSymbolic
phase constant if it's not concrete, which includes PeriodSelfSymbolic,
because we don't want to evaluate the Call expr until the `.Self` gets
replaced during identify. But that means the Call constant value, and
its operands including PeriodSelfSymbolic constants, all end up in the
eval block. We fix this by making Call evaluate to a PeriodSelfSymbolic
phase if its input phase is PeriodSelfSymbolic. This keeps the RHS of
the `where` expression self-contained. If the facet type is concrete,
then it doesn't add any instructions to the enclosing eval block (which
may not exist at all in a non-generic context). Regardless, when we
identify the facet type and replace `.Self`, the dependent Call
expression will be rebuilt as well and it will evaluate as appropriate.
We also enclose the `where` expression in its own generic scope. This
means that instructions which are not part of the facet type do not end
up in an eval block. They may be symbolic instructions, and they will
not get updated by a specific when it's applied the enclosing generic
anymore. But since they are not part of the facet type their
substitution is not required.
The new test had an incorrectly elided SpecificImplFunction instruction
in the resulting semir, as SubstPeriodSelf was using the location of the
input canonical instructions as the location for the results which
resulted in an instruction without a location. We fix that by using
`loc_id_` in the subst rebuild.
Formatting now saw the initialization of a value from an `eval fn` call
to a function in a witness table, so it needed to be taught to look
through ImplWitnessAssociatedConstant (which is what we find in the
witness table).
This allows two C++ callers to the same Carbon function, written in
different Carbon files, to generate thunks that won't collide/duplicate
symbol at link time - instead the thunks get deduplicated silently by
the linker.
Lexer diagnostics used a single `const char*` as their source location,
which only allowed pointing at a single character rather than a run of
characters. This limited the underlining of unrecognized tokens, and
also made escape sequence diagnostics particularly strange by stopping
at whatever point the lexer discovered the error and pointing only at
that location. For example: `"\w"` was reported at the `w`, and `"\x"`
at the closing `"`.
This change tries to always keep the primary location of a diagnostic
the point at which typing (inserting or deleting) might fix the error.
So for `"\x"`, `\x` is underlined, but the position is at the `"` as
that's where you would type a hex code.
The lexer's emitter now takes a special lexing source location that
represents either a single position _between_ two characters, a single
character, or a range of the source buffer. The lexer diagnostics are
updated to construct these for the character range as appropriate.
Parser diagnostics for a missing token were located at the _next_ token,
which is particularly confusing for terminators like missing `;`s. Now
they are located at a position after the previous token, with or without
a space depending on the token, but typically on the same line.
Diagnostics still sort by how far the parser has read.
The parser diagnostics for unclosed groups also now attaches a label to
the opening token when it is on a separate line. And the diagnostics for
mismatched indent in a multi-line string attaches a label to the closing
`'''` that establishes that indent.
There are likely more location improvements we should do here, but this
seemed like a reasonable incremental improvement and is what Claude
found when doing an initial audit. One example where I think we should
make further improvements is around suggesting raw identifiers when we
find an unexpected `Cpp` or `Core` or `base`. We do this consistently
now, but raw identifiers should be rare. A TODO is added for these
cases.
Assisted-by: Claude Code
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
There were a number of iterations on the design that weren't reflected
in the initial port to the terminal rendering library. This PR tries to
complete the rendering side of things to match the design we landed on:
code-focused, with messages attached by lines to the code, and condensed
as much as possible to avoid the vertical space.
It also improves the styling to make things more visually
distinguishable such as adding a `->` to the main error line, and making
the underlines and connectors for it have a heavy weight (when the font
supports that).
Another major change is having multiple messages on a single line be
drawn in source lexical order. This requires the connectors to cross
without joining, but makes it easier to read the messages.
This PR also has some refactorings to enable these improvements, and a
_number_ of improvements to comment prose and terminology to try and
avoid jargon and make things read more naturally. Sorry for the fact
that Claude left a bunch of this mannerly language in comments despite
my efforts, but I'm continuing to try and drive the prose towards
something direct and simple to read. Some of the terminology
improvements also needed to be made in the design doc and those have
been made here.
Note that I don't think this rendering is perfect or should be viewed as
final. I'm happy to iterate on it further, and have a few adjustments in
mind already, but would like to focus this on getting the
_implementation_ of the current rendering design in place, and then we
can make targeted changes or tweaks as needed. For example, I'm not yet
fully happy with the rendering when messages are too long and we move
them all the way to the left, but am leaving that for a follow-up.
Assisted-by: Claude Code
Implements check and pattern matching support for struct patterns.
This PR implements complete patterns, and leaves `_` field handling for
future work.
Add access tests for private fields and methods, in both regular classes
and generic.
Modify `BuildCppToCarbonThunkDecl` to properly set the access rather
than always using public.
Also add `static` to a method that isn't used outside export.cpp.
The rendering we're trying to implement based on
`toolchain/docs/diagnostics_rendering.md` needs some features the
initial terminal API didn't support:
- Heavy stroke weight on lines
- Vertical lines that cross earlier lines without joining them
- Wrapping that keeps quoted code whole
This PR adds support for these and removes the two `buffer.h`
TODOs that described them.
This PR also tries to improve the ASCII fallback quality a bit by using
`^`-lines for "heavy" weight, and using `.` and `'` when drawing tees.
Still not perfect of course, but seems reasonable given the constraints
of ASCII rendering.
Assisted-by: Claude Code
Previously, it was possible for rounding to result in small percentage
constructs to be dropped entirely, breaking invariants. For example,
when generating >100 entities, a 2% share should reliably be >=2.
Some parts of source gen already propagated remainders to avoid exactly
this issue, but we weren't propagating them everywhere, notably across
depths and different definitions in plans.
We fix this by introducing a helper for this more robust form of percent
scaling that carries remainders across the slices. It also seeds this
rounding from Phi across [0,100) to keep the edge cases uniformly
distributed without causing instability from the source generator's
seed. The seeded remainder doesn't impact providing the invariant, so
the fix should be durable.
Once this is done, we can shard the tests without the separate processes
allowing the above failure mode to occur.
Assisted-by: Antigravity with Gemini
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Carbon gives the C++ declaration of an exported Carbon thunk an asm
label that
includes the target's global symbol prefix (`_` on Mach-O). Clang names
a
declaration with an asm label `\01<label>` in the IR, so that LLVM uses
the
label verbatim as the symbol name, while Carbon lowered the thunk
definition
under its unprefixed name. Both name the same symbol, but they were
distinct
globals in the IR, so LLVM couldn't see that the C++ calls refer to the
Carbon
definition. That only works while the definition is never discarded.
Mark the Carbon thunk as a `CppThunk` (as described in
`toolchain/docs/check/cpp/thunks.md`), record its C++ declaration in
`clang_decls`, and when lowering the thunk, ask Clang for its function
(creating it if necessary) and define the thunk in that function. This
means
that the thunk and the C++ calls to it always use the same IR global,
with
Clang's name and signature.
The thunk takes all of its parameters by reference, so Clang lowers its
parameters the same way Carbon does, but some C++ ABIs (such as Apple
arm64)
return `this` from constructors. In that case, return the parameter that
Clang
marks `returned`.
On targets without a global prefix, the IR names already matched, so the
only
change is that the thunk definitions now carry Clang's parameter and
function
attributes.
We need to zero-initialize the `token_payload_` bits in order for the
normal creation to just be zeroing the whole structure. We do this only
when MSan isn't enabled so we don't lose bit-accurate bug detection.
Assisted-by: Antigravity with Gemini
Instead of using a table to select a bool parameter, this embeds the
first character as a template parameter and uses `if constexpr` in the
dispatch body to only test multi-character symbols with that start with
that character.
Also template the one-char symbol lexing dispatch routines to remove
runtime lookup in the bool table, and use constexpr property queries of
the token kinds.
Last but not least, add a table to precompute a table at compile time to
synthesize `LexError` based on the last byte.
Benchmark results (./scripts/bench_runner.py --runs=12,
-march=x86-64-v3):
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/65536: CPU Time:
-5.297%
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/262144: CPU
Time: -4.936%
- BM_RandomSource: CPU Time: -4.475%, tokens_per_second: +4.684%
- BM_GroupingSymbols/32/32/32: CPU Time: -3.215%, tokens_per_second:
+3.321%
Assisted-by: Antigravity with Gemini
This moves the fast path of several functions into the header, and
extracts and sinks the slow paths into cold-attributed out-of-line
functions. The result is that the large cold code is separated and the
fast path is easily inlined into callers.
Also does some factoring to have the simplest and most minimal dispatch
to the cold paths throughout.
Benchmark results (./scripts/bench_runner.py --runs=12,
-march=x86-64-v3):
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Parse>/16384: CPU
Time: -7.285%
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Parse>/65536: CPU
Time: -6.995%
- BM_CompileImplFile<Lang::Carbon, Phase::Parse>/16384: CPU Time:
-7.341%
- BM_CompileImplFile<Lang::Carbon, Phase::Parse>/65536: CPU Time:
-5.805%
Assisted-by: Antigravity with various models
Currently, these lookup the token in its `ValueStore`, and then access
the kind and flag from that. This is near-optimal in the lexer while
_writing_ the tokens, but ends up being considerably slower in parsing
when the overhead of digging the token out of the value store dominates
the actual use case.
This PR does an struct-of-arrays reorganization of the kinds and
whitespace flags, but only after lexing. This lets lexing write a linear
series of info structs, and only afterward re-organize into a
query-optimized representation that is a struct of arrays.
However, to make this a net performance gain, we have to create the
arrays _very_ efficiently. We can avoid hand-written assembly here by
very carefully and precisely writing the inner loop to build these
arrays so that LLVM will vectorize it into high-quality SIMD code for us
across both x86 and Arm. We also need the arrays to have a particular
shape (for example, using a `SmallVector<bool>` rather than a
`BitVector`) in order to preserve this vectorized creation. It burns a
tiny amount of memory, but is key to the whole thing being a net win of
5% to 10% across parsing.
Benchmark results (./scripts/bench_runner.py --runs=12,
-march=x86-64-v3):
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Parse>/16384: CPU
Time: -9.045%
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Parse>/65536: CPU
Time: -6.526%
- BM_CompileImplFile<Lang::Carbon, Phase::Parse>/16384: CPU Time:
-9.275%
- BM_CompileImplFile<Lang::Carbon, Phase::Parse>/65536: CPU Time:
-8.382%
Assisted-by: Antigravity with both Gemini and Claude
A number of stores have been added to File without updating
CollectMemUsage. Add all the stores that were missed, ordering
everything by the order the fields appear in the class. Also include the
filename_ string, which requires a new overload of Collect, since
strings are like vectors of chars.
These only show up post-merge as we're only running ASan there.
This disables the recent `[[clang::preserve_none]]` optimization when
ASan is enabled as there is an LLVM bug on AArch64 that causes register
corruption with that calling convention. This is factored into a common
helper library so we can use it in other places as well.
Assisted-by: Antigravity with Gemini
This test generates an infinite recursion between Carbon and C++ making
a new type at each step through the recursion, producing an infinite
number of types. We improve the test in a few ways:
- Remove direct dependencies on min_prelude parts, we should just depend
on a single min_prelude, and not `import Core` explicitly.
- Remove a dependency on `i32` so we rely on less prelude.
- Remove the use of pointers, which currently introduce a dependency on
`Optional`, so we rely on less prelude. Instead use a generic `Wrap(T)`
type that we nest repeatedly to generate types.
- Add some comments and expand function names to help explain the
behaviour being tested
Now the test just depends on the convert.carbon prelude, which makes it
much simpler when running under a debugger.
This adds free function declarations to the top of the source and
adjusts many of the synthesized expressions in the benchmark source to
emit calls to them. This gets the distribution of expressions in our
generated definitions to be significantly more realistic compared to
existing C++ function bodies here and in LLVM.
This still doesn't get us to start calling more methods, have class
types in the signatures of the calls, or have more interesting call
graphs, but seems like a reasonable first step and still an improvement
in the overall fidelity of our benchmarks.
Illustrative generated Carbon:
```carbon
fn Abcde(x: i32, y: i64*) -> (bool, i64);
fn Fghij(p: i32*) -> i32;
// ...
fn MyClass.Method(self, p: i32) -> i32 {
var acc64: i64 = 0;
var acc: i32 = acc64 as i32;
acc = acc + self.Checksum() + p + 1;
let (abc: bool, def: i64) = Abcde(0, &acc64);
acc = (acc + (if abc then 1 else 0) + (def as i32)) + Fghij(&acc) * 2;
return 0;
}
```
Assisted-by: Claude Code and Antigravity with Gemini
---------
Co-authored-by: Geoff Romer <gromer@google.com>
For stores with untagged ID types, `Apply` and `Remove` still did (some)
of the work, despite the fact that it could never be needed. Because
both depend on runtime parts of the class, we need to explicitly check
whether the store's IDs are tagged at compile time with `if constexpr`.
This is important because the hottest value stores aren't tagged, and we
get significant performance uplift in lexing and parsing.
Benchmark results (./scripts/bench_runner.py --runs=12,
-march=x86-64-v3):
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/65536: CPU Time:
-4.713%
- BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Parse>/65536: CPU
Time: -7.251%
- BM_CompileImplFile<Lang::Carbon, Phase::Lex>/65536: CPU Time: -8.016%
- BM_CompileImplFile<Lang::Carbon, Phase::Parse>/65536: CPU Time:
-9.477%
Assisted-by: Antigravity with various models
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
This uses SSE2 to optimize the horizontal space skipping path when there
is more than a single character. That helps with indentation and other
edge cases and doesn't cost much because we only begin when >1
horizontal space to skip.
We use SSE2 here which is available ubiquitously as AVX2 doesn't provide
any significant advantage.
Agy's benchmarks (`./scripts/bench_runner.py --runs=12`, `-c opt
--copt=-march=x86-64-v3`, vs. `trunk`):
- `BM_HorizontalWhitespace/16`: -4.610% CPU time, +4.832% bytes/s &
tokens/s
- `BM_HorizontalWhitespace/64`: -45.977% CPU time, +85.106% bytes/s &
tokens/s
- `BM_HorizontalWhitespace/128`: 0.428x CPU time, 2.338x bytes/s &
tokens/s
Assisted-by: Antigravity with Gemini
---------
Co-authored-by: Geoff Romer <gromer@google.com>
The `memchr` approach didn't really fully capture what we can do with
AVX2, so build a dedicated optimized path for it.
The optimized path does carefully optimized 64-byte block scanning and
decodes the newline positions. I've tried to ensure there are enough
comments to understand the different micro-optimizations that went into
it getting the full performance potential.
My agent's benchmarks (`./scripts/bench_runner.py --runs=12`, `-c opt
--copt=-march=x86-64-v3`, vs. `trunk`):
- `BM_BlankLines/16`: -11.158% CPU time, +12.557% bytes/s & lines/s
- `BM_BlankLines/64`: -21.623% CPU time, +27.587% bytes/s & lines/s
- `BM_BlankLines/128`: -26.332% CPU time, +35.742% bytes/s & lines/s
- `BM_CommentLines/128/0/0`: -22.198% CPU time, +28.531% bytes/s &
lines/s
Assisted-by: Antigravity with Gemini
Determining that a type has trivial destruction requires checking
whether all of its subobjects have trivial destruction and whether its
type has a custom witness for `Destroy.Op`. This isn't possible when
`Destroy` is the custodian for `Destroy.Op` and `SubobjectDestroy.Op`,
so we need to make `SubobjectDestroy` its own interface.
`Destroy.SelfDestruct` becomes a standalone function because it depends
on `SubobjectDestroy` and `Destroy` being implemented, and is also
responsible for generating any custom witnesses for both interfaces.
Making `SelfDestruct` an interface was experimented with, but this
required a blanket impl that caused issues for facet types.
`Destroy` depends on `SubobjectDestroy` to be implemented. We'd ideally
use `require impls SubobjectDestroy`, but it isn't complete yet, and it
also causes problems when `Destroy` is implemented inside the class
definition. We work around this by temporarily renaming the `Destroy`
interface to `DestroyImpl`, and introducing a constraint called
`Destroy` so that user code doesn't need to write `Core.SubobjectDestroy
& Core.Destroy`.
`FinishAst` resolves C++ template instantiation. Template instantiation
can complete Carbon types, generate thunks, and call generic functions.
These operations can add new entries to `definitions_required_by_use()`.
Similarly, resolving any of those can generate more C++ templates that
need instantiating. This change moves `FinishAst` ahead of
`CheckRequiredDefinitions` and loops until no new Carbon definitions
have been added. This prevents later passes from crashing when
evaluating specifics on unresolved generics or uninstantiated templates.
`FinishAst` also disconnects the AST. Subsequent definitions and `impl`s
may depend on C++ interop, so the AST must remain connected until all
the Carbon end-of-file checks complete. We achieve this by decoupling
AST disconnection from finalisation.
Assisted-by: Antigravity with Gemini
Assisted-by: Antigravity with Claude Opus 5.5 Max
Add `BM_CompileApiFileRealisticHeader`, which models a typical C++
header. Each class defines 2 getters, 1 predicate, 2 forwarders, and 2
function definitions inline, and declares 2 methods along with the
private methods that the forwarders call. The dense declarations
benchmarks keep modeling a declaration-heavy API file.
The counts come from classifying, with libclang, the member functions
defined in class definitions in the hand-written headers that the
toolchain includes: 1,612 in the Carbon toolchain's headers, 10,406 in
LLVM's, and 13,366 in Clang's. Classified the same way, the generated
C++ defines 69% of its member functions inline in the class definition,
against 61-70% in the real headers. 78% of its inline definitions are a
single statement, against 72-82%, and 21% return the result of a member
call, against 19-21%. Only 2% have control flow, against 7-13%, because
most inline function definitions have only the three statements that
every plan-based definition has.
Assisted-by: Claude Code and Antigravity with Gemini
---------
Co-authored-by: Geoff Romer <gromer@google.com>
The primary performance challenge with `musttail` dispatch is register
allocation. We can use the `[[clang::preserve_none]]` calling convention
to address this, dramatically improving the register pressure across
these dispatched routines.
However, to be effective, we need to very carefully manage inlining to
keep out-of-line functions from breaking the ideal register allocation,
but keep the right functions out-of-line. A bunch of annontations are
added to push things in one direction or another.
This also requires us to move error handling out of the tight dispatch
loop in more places, for example by returning `LexResult::NoMatch()` and
letting the outer loop handle it.
Agy's benchmarks (`./scripts/bench_runner.py --runs=12`, `-c opt
--copt=-march=x86-64-v3`, vs. `trunk`):
- `BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/65536`: -9.594%
CPU time
- `BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/262144`:
-7.786% CPU time
- `BM_ValidIdentifiers<3, 5, true>`: -9.194% CPU time, +10.125% bytes/s
& tokens/s
- `BM_RandomSource`: -5.062% CPU time, +5.332% bytes/s, lines/s &
tokens/s
Assisted-by: Antigravity with Gemini
Adds support for exporting both the generic function decl with concrete
default arguments to clang, as well as exporting the monomorphized
concrete function def.
This required adding the `SpecificId` to the exported global variable
name, to avoid duplicate names in the global vars. I also now set up an
evaluated context in `clang_sema` when declaring the default arg
expression with clang, to avoid the problem of emitting concrete AST
expressions in an unevaluated context from the in-progress template
function decl context when exporting default values for generics.
Note: Default values for generic parameters of generic functions are not
yet supported.
Extract the allocation path (which is inherently cold due to growth).
Making the outlining effective requires both blocking further inlining
and using `preserve_most` so that the existence of the call doesn't
disrupt the register allocation.
We also mark the insertion statements with callbacks as `always_inline`
so it being cold doesn't cause it to be left out-of-line and further
disrupt register allocation. It's a bit strange to force inlining of a
cold-path function, but the register impact of the call extends back
into the hot-path.
Agy's benchmarks (`./scripts/bench_runner.py --runs=12`, `-c opt
--copt=-march=x86-64-v3`, vs. `trunk`):
- `BM_ValidIdentifiers<3, 5, true>`: -6.277% CPU time
- `BM_ValidIdentifiers<3, 16, true>`: -4.525% CPU time
- `BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/262144`:
-2.012% CPU time (47.5 ms ± 6.358% -> 46.54 ms ± 1.773%, p=0.0167)
Assisted-by: Antigravity with Gemini
The `var_name` string was built on the stack and then added to the
`IdentifierStore`. This lead to use-after-free, since `IdentifierStore`
just stores a `llvm::StringRef`.
Add it to `ast_context().Idents` to provide a longer-lived reference.
This re-lands #8001 (reverted in #8009 after accidentally merging with
earlier unmerged PRs in the stack).
Build on the plan-based emission to make function definitions cover a
more representative breadth of the language: class-typed local
variables, and local variables declared with destructuring. This
requires some complexity to initialize and manage the class type
variables, but ultimately should make all of our compile time benchmark
data more representative by covering a more realistic breadth of the
language surface.
Claude's rough sampling of Carbon, Clang, and LLVM source suggests class
types range from 30% to 80% of the locals, and structured bindings range
from 0.2% to 2%. When synthesizing, we target approximately 50% of
declarations as class types and 2% as destructurings as we anticipate
destructuring to be of growing popularity and so want to ensure we're
measuring any overhead there.
Assisted-by: Claude Code and Antigravity with Gemini
Co-authored-by: Geoff Romer <gromer@google.com>
If a generic class inherits from a private parent (as allowed in C++),
and attempts to access a member of that private parent, an error should
be diagnosed via `DiagnoseInvalidQualifiedNameAccess`.
A couple changes are needed to make that work, because the access
constant in this case is not a `ClassType`. Instead, it's a
`StructValue` of type `ParameterizedClassType`. So, use
`TryGetTypeIdForTypeConstantId` instead of `GetTypeIdForTypeConstantId`
(to get `None` instead of asserting on a non-type constant), and allow
getting the class ID from both `ClassType` and `ParameterizedClassType`.
---------
Co-authored-by: Christopher Di Bella <cjdb.ns@gmail.com>
Drop dependencies on the prelude so it can be debugged with no prelude
import if needed. Ensure the return type of MakeC is used in C++ since
it's part of the imported function signature.
We use the dry-run mode for running benchmarks in a test. This is
important, but doesn't need to use large batches of files. Doing so
would cause the compilation benchmark to become excessively slow,
especially on GitHub runners.
Sadly, Google Benchmark doesn't expose its dry-run mode, but we already
have our own `main` function that injects custom logic for our
benchmarks. Extend that to detect the dry run flags and environment
variables so that we can customize the benchmark behavior in that mode.
Then we use that detection to reduce the batch size.
Also cleans up the counters computation to both be a bit better
organized (I think), and to avoid keeping the tokenized buffers we build
to count tokens around during the timed running of the benchmark.
Also cleans up a bug in some clamping code spotted while fixing this.
Assisted-by: Antigravity with Gemini
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Richard Smith
Nicholas Bishop
Lucile Rose Nihlen
Chandler Carruth
oli-ej
Dana Jansens
Christopher Di Bella
Nicholas Bishop