This proposal codifies our preference for designs that support
"progressive
disclosure", meaning that programmers can ignore a given language
concept (or
even be unaware of it) until it is directly relevant to the task they're
doing.
These have unique challenges for our hashing scheme, and so its useful
to make sure the hash functions we use can handle them.
Some other work on Abseil's hash tables uncovered that this might be
risky and may have surfaced some improvements to reduce the impact here,
but the first step seems to try and start covering this path in the
benchmarks.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I've been mulling this mainly for the parameter complexity of
check/lower, but doing lex/parse for symmetry.
I'm motivated by the plan to move dumping for all of them into the
respective functions, because of discussion about llvm-verifier. That
basically would add another bool parameter (or more) to each of these.
My instinct is we're going to probably accrue a little more over time,
so I'm suggesting this as maybe adding the boundary a little simpler
and/or easier to read.
Note it may make sense to refactor a little further, e.g. maybe
Lower::Context could receive the full set of options and pick out what
it wants, but I figured creating the struct itself would be a decent
start.
I'm trying to put things into options when we can produce a reasonable
default if the user doesn't assign a value. I'm using an explicit
constructor so that values can be added without affecting every caller.
A different factoring would be to pass in everything through the param
struct, but that just felt weird when I was trying it out.
Removing `inst_namer` and `module_name` from `LowerToLLVM` params --
both of these can be inferred from `sem_ir`, and I'm not seeing a
particular reason to maintain them at the call site.
This changes `Destroy` to use an interface for its implementation.
Note that this change includes a lot of test updates. Even when
`Destroy` is a no-op, it still causes code generation as part of
determining that.
Originally I was trying to use ranges to cut down the scope of this, and
to a degree I think they have. But a flipside here is that cases where
no destructors should be generated -- particularly globals -- would be
needed to completely remove destructor calls. Even for ranges, the range
can often include the destructor placement. So I've shifted
frame-of-thought a little: accept a bunch of destructor churn, because
destructors are needed and will be prevalent. The verbosity is a feature
of the design to make desugaring apparent in IR, not a bug.
The `ClangDecl` struct caused some confusion here -- it is embedding
extra data into a `CanonicalValueStore` that isn't used for lookups or
canonicalization, but is useful to store along side. This changes the
`CanonicalValueStore` to support customized key type for `Lookup` so
that we can provide the more direct API that only takes the relevant
key.
This in turn takes advantage of the support for heterogenous keys in the
underlying `Set` as long as hashing and equality are consistent. We do
need to add support for heterogenous equality comparison with
`clang::Decl*`, but that is fairly easily done now that the
argument-reversed form isn't needed as well.
Lastly, this cleans up the `ClangDecl` customization points to be more
idiomatic by using `operator==` and `CarbonHashValue`. While there, I've
added comments to make it unambiguous why we can use the pointer value
for the underlying `clang::Decl` due to the Clang AST's
address-as-identity model.
Resolves the immediate TODOs around this type.
Future work might involve changing from the current `Add` API to one
more like `Map` and `Set`'s API where a callback is used to create the
object, but that level of API complexity isn't necessarily motivated yet
and can easily be a follow-on if and when its worth doing. The `Add`
code paths *are* working with the `inst_id` in order to create an
instruction if we are importing the Clang declaration. It is the
`Lookup` code paths that never needed to know about the `inst_id` and
became more confusing for having to stub it out in the API.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
The goal was/is to reduce the overhead for vtables in generics - the
previous representation/prior to this patch caused a new vtable to be
created in every specific which isn't generally what we want for Carbon
generics (the whole specific/generic thing is meant to avoid creating
specific versions for things that can be a generic form parameterized by
a specific instead of manifest as a unique entity per specific)
So this moves vtables to a top level object (like functions, classes,
etc). Each dynamic class will have a vtable in this list.
Classes have a `vtable_ptr` instruction in them that points to the
vtable.
The actual generic support hasn't been implemented in this patch, as
I've been struggling with just getting this part of the migration going
& wanted to get it flushed out before adding the additional
complications.
It's possible more laziness when doing cross-file importing would be
suitable - for instance if we only need to reference the vtable from
another file, but don't need to know its individual contents, it may be
beneficial for the functions in the vtable to be import_refs (or to add
another layer of indirection - so it can be a single import_ref
all-or-nothing for the functions in the vtable).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This adds something similar to the level of `const` support - that it's
a type, but not the conversions and limitations on usage that are
needed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Suggested by zygoloid while looking at #5678
```
CHECK failure at toolchain/lower/context.cpp:62: !llvm::verifyModule(*llvm_module_, &errs): Verifier errors: Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem0.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 0, !dbg !13
Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem1.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 1, !dbg !13
Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem2.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 2, !dbg !13
```
Adds a `--llvm-verifier` flag to be able to turn this off easily,
particularly for debugging the LLVM IR.
The call workaround is due to a verifier requirement `inlinable function
call in a function with debug info must have a !dbg location`. It
specifically comes up for the `++x` case, with `%1 = call i32
@"_CConvert.8b3d5d6a6c17be04:ImplicitAs.Core.b88d1103f417c6d4"(i32
%other)`. I think #5397 is in the direction of a fix for that, but #5397
was set aside because it puts the debug info in too many places.
Instead, address this by adding a stub location for calls that don't
have a good location. I'm deliberately putting this next to the TODO so
that it's easier to understand the association.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Add check support for `for` loops following #1885. This also adds a
basic `Optional` type to the prelude, as that's necessary to support the
new `Iterate` interface.
Depends on #5688, #5697. Those PRs aren't stacked here, but this change
will crash until they land.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Previously we created allocas for temporaries at whatever point in the
output LLVM function we'd reached. This would result in these being
dynamic allocas (performing a dynamic stack allocation), which is
inefficent and can lead to a stack overflow if it happens in a loop.
Switch to putting the allocas in the entry block instead, and instead
generate a lifetime start marker when we reach the point where the
temporary is introduced. We already did this for local variables; this
is just factoring out and reusing that code.
In a facet type constraint, you can write `where .Self impls T` for any
facet type `T`, or the constant `type`. It is possible to write `type`
in different ways though, with a `NameRef` instruction appearing on the
RHS instead of `TypeType`. In this case, the canonical constant value's
instruction will still be `TypeType`, so make eval look at the canonical
instruction to see this.
Add a test with an `alias Type = type` which hits this case.
After this change, we only will accept and find one of the following on
the RHS of `impls`:
- `TypeType`
- A facet type
- An error, if the source code had something else there, which will
already be diagnosed. Tested by `fail_right_of_impls_non_type.carbon`
and `fail_right_of_impls_non_facet_type.carbon`.
So we handle these three cases, and drop the implicit handling of other
things which will never appear there.
For some of these, it's just replacing with min_prelude/none.carbon.
Some had min_preludes specified, and I'm generally switching those to
none.carbon as well. The one exception is the destroy.carbon test, which
I noticed because of #5678
`AddImportedInstruction` was turning errors in an instruction into a
Runtime constant value instead of an Error, which led to crashes when
importing an instruction that had an error inside it somewhere.
Fixes#5726
The `IsPeriodSelf` function is problematic, as it's possible for a
FacetType to contain multiple `.Self` bindings which refer to different
selves, when one FacetType is nested within another: `I where .Self.J =
(K where .Self impls type)`.
The `WhereExpr` instruction contains the instruction of the `.Self` of
that `where` clause, which is what `IsPeriodSelf` is looking for, so we
can compare with its constant value instead.
- Update all the llvm::Function pointers after function replacement.
Some were previously left in an inconsistent state.
- Only do function replacement once, after converging on the canonical
specific to use.
The `full.carbon` prelude just sets a flag indicating an explicit intent
to include the full prelude. Once all tests include some prelude file,
an error can be enabled (currently it's commented out) that requires an
`INCLUDE-FILE` of some min-prelude to be present in all `check/` and
`lower/` file tests.
Update remaining parts of lowering, in particular the lowering of
aggregates, to handle lowering within a specific from a different file
than its generic. Look up information about a type in the current
specific and in its file rather than performing lookups for the type in
the generic and its file.
Remove or fix all remaining uses of raw `TypeId` in
lower/function_context and lower/handle*, so that the type from the
specific is consistently always used when lowering a specific function.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This script runs benchmarks written using Google Benchmark repeatedly,
and collects the results from JSON to render them nicely and provide
statistical information across the runs.
Because this runs the binaries repeatedly, this can help account for
run-to-run variations that are pervasive in many of Carbon's benchmarks,
such as ASLR and other process-specific differences.
It's most basic mode runs a benchmark multiple times and shows both
median and confidence intervals.
It also supports two comparison modes:
1) Regular expressions can be provided that describe collections of
related benchmarks where one is the "main" benchmark and the others
are comparable. For example, Carbon's data structure vs. data
structures from LLVM or Abseil. These will be rendered with the main
benchmark first, followed by a comparison relative to a "baseline" of
each comparable benchmark.
2) A baseline benchmark binary, and potentially different command line
flags, can be provided to run two benchmark binaries and compute
a comparison for each benchmark within them.
Across all of these, the script works to present the best text UI it can
in the console. I may have gotten a bit obsessed with rendering the
benchmark results in a way that is really pretty. There are lots of
fancy color coding and progress bars, etc., when run in in the terminal.
For the basic mode without any comparisons, the results look like:
```
Computing statistically significant deltas only wherethe P-value < 𝛂 of 0.05
Metric key:
BenchmarkName... <median> ± <% at 95th conf>
Benchmark ┃ CPU Time ┃ bytes_per_second
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━
BM_LatencyHash<RandValues<uint8_t>, CarbonHashBench>. │ 3.051 ns ± 2.721% │ 327.8 M ± 2.765%
BM_LatencyHash<RandValues<uint8_t>, AbseilHashBench>. │ 3.395 ns ± 4.377% │ 294.6 M ± 4.572%
BM_LatencyHash<RandValues<uint8_t>, LLVMHashBench>... │ 6.125 ns ± 2.662% │ 163.3 M ± 2.726%
BM_LatencyHash<RandValues<uint16_t>, CarbonHashBench> │ 3.105 ns ± 3.947% │ 644.1 M ± 4.109%
BM_LatencyHash<RandValues<uint16_t>, AbseilHashBench> │ 3.433 ns ± 4.308% │ 582.6 M ± 4.502%
BM_LatencyHash<RandValues<uint16_t>, LLVMHashBench>.. │ 6.127 ns ± 2.540% │ 326.5 M ± 2.587%
BM_LatencyHash<RandValues<uint32_t>, CarbonHashBench> │ 3.082 ns ± 2.846% │ 1.298 G ± 2.923%
BM_LatencyHash<RandValues<uint32_t>, AbseilHashBench> │ 3.401 ns ± 3.611% │ 1.176 G ± 3.739%
BM_LatencyHash<RandValues<uint32_t>, LLVMHashBench>.. │ 6.209 ns ± 4.064% │ 644.3 M ± 4.236%
BM_LatencyHash<RandValues<uint64_t>, CarbonHashBench> │ 3.122 ns ± 2.871% │ 2.563 G ± 2.956%
BM_LatencyHash<RandValues<uint64_t>, AbseilHashBench> │ 3.426 ns ± 2.811% │ 2.335 G ± 2.892%
BM_LatencyHash<RandValues<uint64_t>, LLVMHashBench>.. │ 6.497 ns ± 3.081% │ 1.231 G ± 3.179%
```
For the first comparison mode on one of Carbon's benchmarks, the results
look like:
```
Computing statistically significant deltas only wherethe P-value < 𝛂 of 0.05
Metric key:
BenchmarkName... <median> ± <% at 95th conf>
vs Comparable: 👍 <delta> p=<U-test P-value>
<median> ± <% at 95th conf>
Benchmark ┃ CPU Time ┃ bytes_per_second
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━
BM_LatencyHash<RandValues<uint8_t>, CarbonHashBench>. │ 3.037 ns ± 1.781% │ 329.2 M ± 1.813%
vs Abseil: │ 👍 -8.200% p=0.000183 │ 👍 8.933% p=0.000183
│ 3.309 ns ± 2.064% │ 302.2 M ± 2.022%
vs LLVM: │ 👍 -49.401% p=0.000183 │ 👍 97.632% p=0.000183
│ 6.003 ns ± 1.502% │ 166.6 M ± 1.480%
│ │
BM_LatencyHash<RandValues<uint16_t>, CarbonHashBench> │ 3.026 ns ± 1.816% │ 661 M ± 1.784%
vs Abseil: │ 👍 -8.599% p=0.000183 │ 👍 9.408% p=0.000183
│ 3.311 ns ± 1.873% │ 604.1 M ± 1.839%
vs LLVM: │ 👍 -49.829% p=0.000183 │ 👍 99.319% p=0.000183
│ 6.031 ns ± 2.806% │ 331.6 M ± 2.730%
│ │
BM_LatencyHash<RandValues<uint32_t>, CarbonHashBench> │ 3.017 ns ± 2.696% │ 1.326 G ± 2.625%
vs Abseil: │ 👍 -9.754% p=0.000183 │ 👍 10.808% p=0.000183
│ 3.344 ns ± 1.537% │ 1.196 G ± 1.514%
vs LLVM: │ 👍 -49.857% p=0.000183 │ 👍 99.427% p=0.000183
│ 6.018 ns ± 3.269% │ 664.7 M ± 3.167%
│ │
BM_LatencyHash<RandValues<uint64_t>, CarbonHashBench> │ 3.025 ns ± 3.395% │ 2.644 G ± 3.284%
vs Abseil: │ 👍 -9.812% p=0.000183 │ 👍 10.879% p=0.000183
│ 3.354 ns ± 2.640% │ 2.385 G ± 2.572%
vs LLVM: │ 👍 0.476x p=0.000183 │ 👍 2.101x p=0.000183
│ 6.357 ns ± 2.477% │ 1.258 G ± 2.418%
│ │
```
For the second mode, in this case comparing a baseline build with `-Oz`
vs an experiment with `-Os`, the results look like:
```
Computing statistically significant deltas only wherethe P-value < 𝛂 of 0.05
Metric key:
BenchmarkName... 👍 <delta> p=<U-test P-value>
baseline: <median> ± <% at 95th conf>
experiment: <median> ± <% at 95th conf>
Benchmark ┃ CPU Time ┃ bytes_per_second
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━
BM_LatencyHash<RandValues<std::pair<uint32_t, uint32_t>>, CarbonHashBench> │ 👍 -35.870% p=0.000557 │ 👍 55.930% p=0.000557
baseline: │ 5.704 ns ± 1.877% │ 1.403 G ± 1.911%
experiment: │ 3.658 ns ± 4.209% │ 2.187 G ± 4.039%
│ │
BM_LatencyHash<RandValues<std::pair<uint32_t, uint64_t>>, CarbonHashBench> │ 👍 -19.475% p=0.00119 │ 👍 24.186% p=0.00119
baseline: │ 4.974 ns ± 3.029% │ 3.217 G ± 3.124%
experiment: │ 4.005 ns ± 4.297% │ 3.995 G ± 4.120%
│ │
BM_LatencyHash<RandValues<std::pair<uint32_t, int*>>, CarbonHashBench>.... │ 👍 -11.740% p=0.00153 │ 👍 13.302% p=0.00153
baseline: │ 4.634 ns ± 3.433% │ 3.453 G ± 3.555%
experiment: │ 4.09 ns ± 2.999% │ 3.912 G ± 2.911%
│ │
```
The script itself uses a new tool for managing dependencies called `uv`:
https://docs.astral.sh/uv/ This tool allows for the script to contain an
inline set of dependencies that will be installed and cached for
subsequent runs. This seemed particularly important as dependencies like
SciPy and NumPy can be particularly difficult to manager or keep
installed in other ways, but are essential to this scripts statistical
analysis. So far, the `uv` system has been working remarkably well for
me and been a relatively pleasant experience on the whole.
I have included as much of the Python dependencies as have good type
information into the MyPy configuration to get good type checking in
pre-commit however.
Last but not least, this has been a pet project of mine for a quite a
while and so may be a bit rough around the edges as I added and tweaked
functionality based on specific benchmarks I was looking at. It feels
like its gotten useful enough to contribute somewhere, but totally open
to any refactoring or improvements needed. I tried to take a few passes
over it to organize and document the code before sending it, but I'm
sure there are still some things that could use improvement.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This drops file_test runtime from about 3s to 2.5s on my machine, which
is now ~30% faster than before #5653 slowed things down by adding a lot
of stuff to the production prelude.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
If convert fails after applying a substitution in deduce, fail deduction
rather than succeeding deduction with an ErrorInst in the deduced
argument.
For example, in
`toolchain/check/testdata/facet/fail_convert_class_type_to_generic_facet_value.carbon`
the `WrongGenericParam` is deduced for the first argument, then
substituted into the second parameter, but the argument can not convert
to the parameter after substitution. In this case, deduction fails
instead of producig a call with an error in the second argument. The
resulting semir drops the call with an error argument:
```
-// CHECK:STDOUT: %CallGenericMethod.specific_fn: <specific function> = specific_function %CallGenericMethod.ref, @CallGenericMethod(constants.%WrongGenericParam, <error>) [concrete = <error>]
-// CHECK:STDOUT: %CallGenericMethod.call: init %empty_tuple.type = call %CallGenericMethod.specific_fn() [concrete = <error>]
```
This drops file_test runtime from about 2.5s to 2s on my machine, which
is now ~50% faster than before
https://github.com/carbon-language/carbon-lang/pull/5653 slowed things
down by adding a lot of stuff to the production prelude.
This doesn't support actually passing the value of the struct, which is
planned to be implemented using thunks.
`ClangDeclId` value is now `ClangDecl` which includes the mapped Carbon
instruction in addition to the Clang declaration. This allows finding
the Carbon instruction for a given Clang declaration, which is necessary
for mapping a Clang struct parameter type to the Carbon class without
doing name lookup. We don't take the instruction as part of the hash
key, as discussed in
[Discord](https://discord.com/channels/655572317891461132/768530752592805919/1380575881050718469).
To map the type, we also need to map namespaces. To avoid recursion for
inner namespaces, we use a vector.
Note that the first commit just changes the order of functions in the
file to make review easier.
C++ Interop Demo (that shows missing behavior):
```c++
// hello_world.h
struct S {
S(const S&) { x = 1; }
int x;
};
void hello_world(S s);
```
```c++
// hello_world.cpp
#include "hello_world.h"
#include <cstdio>
void hello_world2(S s) { printf("hello_world2: %d\n", s.x); }
void hello_world(S s) {
printf("hello_world: %d\n", s.x);
hello_world2(s);
}
```
```carbon
// main.carbon
library "Main";
import Cpp library "hello_world.h";
fn Run() -> i32 {
var s : Cpp.S;
Cpp.hello_world(s);
return 0;
}
```
```shell
$ clang -c hello_world.cpp
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link hello_world.o main.o --output=demo
$ ./demo
hello_world: -1108224096
hello_world2: 1
```
Part of #5533.
`SubstInst()` replaces individual instructions, and then rebuilds them
into instructions that contain those instructions. If any instruction is
an `ErrorInst`, the final result will also be an `ErrorInst`. In
pathological cases, it's possible to generate large types, [such
as](https://github.com/carbon-language/carbon-lang/issues/5672) tuples
of tuples of tuples of tuples of `something`. If that `something` is
`ErrorInst`, we can save a lot of work by avoiding building the
surrounding types, and evaluating them all to `ErrorInst`.
The none.carbon min-prelude is not just an empty prelude, it also
prevents any prelude from being imported at all. So no import machinery
runs before the test, only the `package` statement from the prelude
would run.
Use the none.carbon min-prelude in a few tests that were specifying
`--no-prelude-import` to give it a trial run.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Jon Ross-Perkins
Geoff Romer
Chandler CarruthandJon Ross-Perkins
dependabot[bot]
Richard Smith
Boaz Brickner
Alina Sbirlea