As part of this, move functions that seem reasonable to make out-of-line
to a separate `_impl.h` header file that is only included where the
explicit instantiation _definition_ is provided.
By using explicit instantiation we can make these templates behave more
like non-template classes in terms of supporting out-of-line definitions
that don't need to be compiled by every translation unit. The set of
eventual instantiations here is fundamentally known, and there tend to
be headers that define a canonical "leaf" type where it makes sense to
trigger the explicit instantiation.
Where we already had a `.cpp` file to put the explicit instantiation
definition, use it. But in some places we didn't have such a `.cpp` file
so this PR adds those.
This also requires that we have precise constraints on APIs that _can't_
be instantiated for specific argument types, as now we don't do this
lazily.
Combined, this appears to reduce the sum of object file sizes in the
`check` directory by almost 40% (122mb -> 74mb) in my measurement.
My actual goal was to improve compile times, but so far I don't have a
great methodology for measuring these... But the object file size
reduction seems to confirm this is a net win and likely represents a
non-trivial improvement in compile time.
Assisted-by: Antigravity with Gemini
## Summary
`ValueStore::GetRawIndex` formatted the first `CARBON_DCHECK` with
`index` before the local `index` is declared. Use `id.index` so the
diagnostic matches the condition being checked.
## Test plan
- `bazelisk build //toolchain/base:base` (or `//toolchain/...` as
appropriate)
The IdTag knows the type of the Id its tagging and the type of the Id
being used as the tag. This prevents mixing up tagged and untagged ids,
and avoids having to work with untyped integers.
Adds an Untagged marker struct that's used as the tag type in IdTag when
no tag is desired.
The complexity of ConstantIds and TypeIds became a bit visible: TypeIds
are concrete ConstantIds. And ConstantIds have two different tagging
schemes, one for concrete and one for symbolic ids. And ConstantIds are
actually re-cast InstIds with the same index. The LoweredTypeStore needs
to work with tagged TypeIds, but the tags actually come from an InstId
store in ConstantValueStore. Now this is expressed in the type system by
getting the tags for TypeIds from the ConstantValueStore.
ValueStores without an TagId type parameter are now visibly untagged.
IdTag is now only default constructible when it does not have a tag,
which means ValueStore is only default constructible when the TagId is
untagged. This forces tagged value stores to be constructed correctly
with a tag at compile time, and untagged ones to be constructed without.
FixedSizeValueStore has overloads for dealing with tagged and untagged
Ids, since it can't default-construct ValueStore for tagged ids, and no
longer requires passing in default-constructed tags when there is no tag
in the ids.
Otherwise the value fails in confusing ways while untagging:
CHECK failure at ./toolchain/base/value_store.h:71:
index >= initial_reserved_ids_: When removing tagging bits,
found an index that shouldn't've been tagged in the first place.
With this change:
CHECK failure at ./toolchain/base/fixed_size_value_store.h:112:
id.index >= 0: instFFFFFFFFFFFFFFFD
Move `GetWithDefault` into the `ValueStore` base class, and avoid doing
the tag -> index mapping twice.
Call `ValueStore::Get` instead of `ConstantValueStore::GetAttached` in
`GetUnattachedConstant`. This is equivalent, since we never need a
default value here, and should be faster and less surprising.
Based on review feedback on
https://github.com/carbon-language/carbon-lang/pull/6215#discussion_r2430177644
There are some intermediate commits with alternatives, finding other
ways (non-templates) to address the layering boundaries between
`ValueStore` construction and `CheckIRId` tagging. But, yeah, template
seems like the way to go - certainly in terms of terseness and probably
in terms of extensibility to other Id tagging as/when needed.
This change makes dumping and debugging work again with InstIds that are
now tagged with the CheckIRId. The textual representation of an InstId
is changed from `irN.instM` back to `instM` but the `M` is now a hex
value with the tag as part of it, which is the same number that is
physically in the `InstId::index` field. This prevents any cases where
we would potentially print incorrect values for large InstIds.
We teach the `dump` command in lldb to parse hex values for InstId so
that we can paste these numbers back into the debugger.
Use the `CheckIRId` as a unique identifier for the scope of an `InstId`
- if an `InstId` is created within the scope of one `CheckIRId` it must
not be used in the scope of a different `CheckIRId`.
This is achieved without extra storage, but with false negatives for
large inputs.
When an `InstId` is created, the original index of the `Inst` is XORed
with a tag derived from the `CheckIRId` to produce the final `InstId`.
When the `InstId` is used, the expected tag is XORed with the `InstId`
to get back to the original index - if the tags don't match, the
resulting index will be corrupted, likely too large - resulting in an
out of bounds index CHECK-failure.
(the tag value is derived as such:
* take the CheckIRId
* left shift one bit (padding zero)
* left shift another bit (padding 1 - used to signify that the resulting
`InstId` has a tag combined into it)
* reverse the bits
In this way, the tag is unlikely to overlap with the index for small
test cases - making it possible to separate out the `CheckIRId` from the
index in these cases to provide more meaningful debugging/CHECK
messages, and more informative `SemIR` textual dumping that can now
include the `CheckIRId` along with the `Inst`'s index in the name of an
`inst`)
The test churn here is improved printing as tagged `InstId`s can now,
with best effort (more likely for small test cases where the `CheckIRId`
and the `Inst` index aren't at risk of overlapping from the high and low
bits), render the `CheckIRId` as part of the inst's name. Going from
`instNN` to `irMM.instNN`.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This is trying to make it clearer when vectors are being indexed with
`CheckIRId`.
The only one that I still kind of want to change is the
`SmallVector<std::unique_ptr<CompilationUnit>>`, but because it's a
`unique_ptr` that's a little more complex. I may not bother.
Note, some of the changes around nuanced `SmallVector` interactions were
based on trying to copy the way `SmallVector` itself takes arguments,
like with range passing.
This changes ConstantValueStore to use ValueStore so that we get the
allocation flow that we're leaning towards there. This adds
`ConstantId::SymbolicId` to represent what was previously an int32
"symbolic_index" (although called an index, it's consistent with ID
usages).
I'm also changing Chunk::at/push to be consistent with the wrapping
Get/Add naming; the naming difference sticks out more with
UninitializedFill being added.
This is reducing ValueStore inference of types from `using`, and removes
`using ValueType = ...` from affected id types.
I'm adding a number of `using FooStore = ValueStore<FooId, Foo>` because
I think it's a little repetitive otherwise; often 4 cases where I'm
doing this: getter, const getter, member, and getter on `Context`. Note
we also have a number of `-> decltype(auto)` that were added I think
mainly to avoid repeating the type, but I'm not sure whether there'll be
agreement on replacing those and so am not changing them here.
I'm placing these aliases with the value type in general, because I
think it's probably easier to view that way. An alternative would be to
put all the types on `File`, but:
- That would be inconsistent with things like `InstStore`, which are
very `ValueStore`-adjacent and put with their value type.
- `File` would have a _lot_ of using's, and the accessors are already
noisy -- I think it would just make the file harder to skim.
Note this is the heart of what I'd brought up [on
Discord](https://discord.com/channels/655572317891461132/655578254970716160/1388199282250613019).
This PR still leaves CanonicalValueStore and BlockValueStore as things
to also add parameters to, but I thought it best to try breaking the set
of changes apart by type. Both of those rely on ValueStore, so
ValueStore needs to change first.
ValueStoreChunk and ValueStoreRange are implemented in a way that's
closely tied to ValueStore, and the separation makes for a lot of
additional template parameter passing, which seems easy to make mistakes
on. Combine types in order to make the close association more implicit.
I also considered passing `ValueT` everywhere, as an additional template
parameter. Note I believe the simplification is important. I'll
highlight four notes that I think favor this approach:
- `ValueStore`, with the chunk type in the same file, now has more of
the closely related implementation features in the same file. I think we
probably will want any chunking to continue to be done by `ValueStore`
itself, with related types using the implementation on `ValueStore` and
never creating their own.
- Making `ValueT` a template parameter on `ValueStore` -- my next step
-- will only change a couple lines of code on this type, instead of
sweeping changes. That should make it easier to be confident of the
correctness of those changes.
- Simpler to verify correctness. For example, `ValueStoreChunk` takes a
`ValueType` parameter that it doesn't forward; other functions assume
they can use `IdT::ValueType`. With the changes, this also no longer
benefits from separating out `IdHasValueType`, which was inconsistently
applied to related types (e.g., `ValueStoreRange` didn't use it).
- Template parameters often lead to `sizeof`, where we can't rely on
type checking to catch mistakes.
- The reduction of code is significant, with 8 `template<...>` removed
(including 1 forward declaration for `ValueStoreRange`), and also the
related `requires`. Correspondingly, places specifying template
parameters also decreased.
As I'm looking at splitting value type setting out, this is to make it a
bit easier to see what's part of each type. Note, I expect
`ValueStoreTypes` to remain because of the `StringRef` logic it does --
I'm giving that its own file.
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>
Changes the vectors on `Lower::FileContext` to be `FixedSizeValueStore`
where possible, which we have several at this point.
This changes `FixedSizeValueStore` to prefer inferring the size from a
`ValueStore<IdT>`, which should make adding incorrect sizes harder. Note
I wasn't sure that adding a `size()` to `TypeStore` that returned
`insts().size()` would be good because it doesn't directly work that
way; `ConstantValueStore` would've also required more work since it
doesn't have access to that right now.
Split out `TokenInfo` to be able to easily write `using ValueType =
TokenInfo;` on `TokenIndex`. Also fixes a small type issue on
`ValueStore` that affected `mapped_iterator` behavior when writing
`old_tokens_it->first < next_offset`.
Trying to build a type around the common idiom we have for types based
on an Id range. The primary advantage of this is it makes clear the `Id`
association, and drops the `.index` use.
Lowering was motivating me because it has a few of these, and check
probably has more (e.g. `tree_and_subtrees_getters`), but I'm just
changing a handful of examples to show the concept and see if there's
agreement.
I wanted to inherit from ValueStoreTypes, but name lookup didn't seem to
find the types without `using` statements, at which point there didn't
seem to be much reason to use inheritance.
This avoids reallocating the backing buffer in ValueStore so that
references into the ValueStore are never invalidated when adding new
values. This works especially well since we never delete values from a
ValueStore.
The strategy used is to allocate chunks of a fixed size, and inserting
into each chunk until it is full before allocating the next. The
ValueStore starts with an initial allocated chunk in all cases, so that
there is only a single indirection for adding and accessing values from
this chunk. After it's full, additional chunks are allocated in a
vector, so two indirections are required to add or access values in
these chunks.
This obviates the need for
https://github.com/carbon-language/carbon-lang/pull/5529 as we no longer
need to worry about holding pointers into a ValueStore.
We introduce a Flatten operation for ranges. It flattens a "range over
ranges over Ts" down to a "range over Ts". This allows us to make an
range over the values in the ValueStore from a range over the chunks in
the ValueStore. See
https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.flatten
for inspiration for this name choice. Flatten is used in one other case
where we were writing two levels of for loops to do the same thing.
The `array_ref()` accessor is changed to `values()` and its now a range
(typed as a `ValueStoreRange`) over all values as references (like
ArrayRef was, but without random access).
As pointers to a ValueStore can no longer be invalidated, we remove the
ASAN poisoning feature and support from ValueStore.
This may cause a regression in our compile benchmark of up to 5%, though
that is close to or within the noise of the benchmark. We can look at
ways to optimize things further in the future. Perhaps by tuning the
chunk size further, or by making later chunks larger than earlier
chunks, or other strategies.
With this enabled, entities that live in value stores are poisoned
whenever any action is taken that might invalidate pointers and
references to those options -- in particular, adding another item to
that value store, or attempting to load any entity from an import IR.
Subsequent uses of those pointers or references then trigger an ASan
failure.
This detects latent bugs where the pointer or reference to the entity
would become stale if we got unlucky about when the value store
reallocates, even in cases where the reallocation didn't actually
happen.
This is not enabled by default: it finds a lot of latent bugs, so our
tests don't pass with this option. This PR also includes fixes for a few
of those bugs.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
`AddDefaultValue` doesn't quite capture the intended semantics; it
should typically be replaced with an actual value when dealing with
control flows. Trying to indicate the "assign later" with
`AddPlaceholder`, mirroring `AddPlaceholderInst`.
Shifting the `protected` functionality on `BlockValueStore` so that it's
not providing functions just for `InstBlockStore` to use. Also hoping
that seeing the comments next to the function name makes them easier to
understand, whereas `using` buries that a little.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
The CompleteFacetType value store is now a RelationalValueStore. This
type of store uses some _other_ id as the key for insertion. It allows
checking if values of the _other_ id are present in the store, since
those ids are handed out by another store and will (necessarily) exist
before there is a matching value in the RelationalValueStore.
The lookup for precense of the _other_ id returns the id of a value in
the RelationalValueStore. That id can be used to get the value out of
the store.
For our use case, the RelationalValueStore maps from FacetTypeId to
CompleteFacetTypeId. So you add a CompleteFacetType to the store with a
FacetTypeId. Then you can query with a FacetTypeId to see if there
exists a CompleteFacetTypeId. And if there is, you can use that
CompleteFacetTypeId thereafter to get the CompleteFacetType value from
the store.
This removes the need for ValueStore::GetMutable() and removes the
method. FacetTypeInfo no longer has a field that needs to be carefully
excluded from hash and comparison.
* Add `RequireCompleteFacetType` and `ResolveFacetTypeImplWitness` to
`check::Context`. Goal was to move code from `impl.cpp` (mostly) without
functional changes.
* Complete type information is cached with the facet type, and is stored
in a `complete_facet_types()` table.
* Main functional change is to diagnose attempts to use a rewrite
constraint on an associated function. Some existing diagnostics have
been updated.
* Remove `check::Context::RequireDefinedType`:
* For class types, use `RequireCompleteType`
* For facet types, use `RequireCompleteFacetType`
* Introduce a `SemIR::SpecificInterface` to hold an interface and
specific id pair.
* Keep the specific interface ids in the impl object.
* Avoid some extra copies in `Dump` functions.
* Future work missing from this PR:
* Resolving for member access or actions that require impl lookup.
* Resolving rewrites constraints that refer to non-concrete values.
* Any support for adding implied constraints that result from a `where`
clause (though TODOs have been added).
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Dana Jansens <danakj@orodu.net>
This allows iterating on all values in a store along with the Id for
each value, instead of `llvm::enumerate(store.array_ref())` which would
give you the indices.
While the indices are really the same as the Ids, this provides a
typesafe way to enumerate() over a store.
There's no use for this right now, but I thought I needed this, and it
helped me debug, and it was a pain to write correctly without dangling
references.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
Use it in the instruction namer to make instruction names more stable
across unrelated changes to the toolchain or the prelude.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Previously Collect() was used for types that implemented
CollectMemUsage() but otherwise Add() was used. This required the caller
to think about the type of the field and know/decide which method to
use.
Now, the caller always uses Collect() unless they are adding specific
byte values, in which case Add is used. Typically then, Add will only be
used to implement the CollectMemUsage() function.
To do this we require all Collect() methods to be templates so that they
all be a single overload set. The Collect on BumpPtrAllocator is
converted to a template that checks
`std::same_as<llvm::BumpPtrAllocator, T>`.
This is in anticipation of making the integer value store be customized
heavily. I'd like to extract it from the common code when doing that, so
first disentangling them here without any intended change in
functionality or behavior to enable that.
I've tried to update `#include`s to be as minimal as I can and added a
few missing includes spotted in the process.
I've split the test for value store to include what was easy focused on
just the value store templates rather than the unified shared value
stores.
This might surface some opportunities for adding more tests, but for
this PR, just doing the minimal restructuring.
When profiling, these jumped out as good inline candidates that happened
to be out-of-line, this just moves them inline so that they're
available. I think as much as 10% improvement in check-phase from this,
but I haven't run detailed before/after measurements as these changes
seemed minimally disruptive.
Also switched from `CHECK` to `DCHECK` in one place that seems
especially hot and where the check itself seems reasonable to only do in
debug builds. Left a comment since we rarely need to remove these any
more.
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>
Most of these are about enabling inlining, in a couple of cases moving
code to a header and throughout switching to `CARBON_DCHECK`. The code
size of `CARBON_CHECK` seems to make inliing quite unreliable. I'm going
to think about whether there are ways to improve this, but a reasonably
small number of these seem worth switching for now to get some compile
time savings.
Also moves VLOG out of the hot path which helps a bit as well.
All combined, this net a bit over 10%, although it varies a bit exactly
how much. We're now pretty consistently over 800k lines/second for check
in the compilation benchmark for files >=4k lines, which makes me happy.
That's remarkably close to our original target.
Not really planning to keep optimizing here, just was glancing at the
profile and many of these stood out to me and were easy to fix.
This uses a heuristic reserve to greatly reduce hashtable growth of the
identifiers hashtable. The design of the hashtable itself is optimized
around compact memory use and is especially slow to grow and so this has
an outsized impact.
The heuristic was computed using `scripts/source_stats.py` and looking
at C++ codebases. We may want to periodically re-evaluate it as Carbon
code emerges and we have better data on its distributions of tokens.
This also required fixing the `Reserve` method on `CanonicalValueStore`
that wasn't actually used anywhere and so didn't even compile correctly.
I added it to the relevant unit test so it is at least compiled locally
to its definition.
This undoes a previous change to unify them, and I think at my advice.
=[ Sorry about that, I think I was just wrong.
Specifically, I think I had suggested that it would be more efficient to
have a single shared hashtable of strings. The more I look at profiles
of the toolchain, the less likely that seems. Specifically for
identifiers and string literals it seems especially problematic.
Using a single, joint hashtable is likely a good idea when all of the
different querying code paths are equally likely, the strings follow the
same distribution of sizes, and either there is no clustering of access
to different sets of strings or none of the sets are meaningfully small
enough to fit into a lower level of resident cache.
I think essentially none of these predicates actually hold for
identifiers vs. string literals:
- Identifiers are *much* more hot
- They have wildly different size distributions.
- The access patterns are very clustered
Sorry for the misleading advice on that one.
While splitting them, I've worked to simplify the code a bit by building
a way to have the `StringRef` holding canonical value stores not require
specializations, and so we get a pretty large code cleanup in the
process here.
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>
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.
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.
Note, I'm annotating the lookup partly so that the reason the conflict
comes up is clear, partly so that there's actually a diagnostic line
associated with the root cause as more tests get packed into a single
file.
This is primarily being done for performance reasons, removing hash
lookups. The increased memory consumption is accepted.
Refactors LexicalLookup out to its own structure.
Namespaces are copied, which means also adding their name to the
underlying instruction. It happened not to be done previously; the name
was only in name lookup.
Since the only import supported right now is the default import,
functionality is limited; in the future I'll need to deal with namespace
vs package conflicts.
Tests of namespace imports are under "namespace" -- I figured this would
be best for scaling as more instructions get support.
This also improves some debugging-related output that I was trying to
use while trying to build the support.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
We have `StringLiteral`s in multiple other `Carbon` sub-namespaces.
Rename to a more specific name to avoid collisions.
We should likely also rename `Carbon::IntId` -> `Carbon::IntValueId` and
`Carbon::RealId` -> `Carbon::RealValueId`, but this collision is
prioritized because it was blocking work on typed parse nodes which
introduces a `Carbon::Parse::StringLiteralId`.
Adds a `LazyImportRef` instruction. Versus `CrossRef`, this is intended
to represent an instruction which cannot be used directly, and must be
replaced when it comes up due to name lookup. The intent is to use this
to avoid recursive loading of imported IR instructions.
Note, under this model, when `ResolveIfLazyImportRef` is called, it
essentially needs to load both inst and type information to a sufficient
point where any further attempts would hit name lookup again. That will
probably be complex, and the current implementation is just touching the
surface of the issue. I was heading down this route because it would
mean we have a limited number of points that need to consider whether
they're going to talk about a `LazyImportRef`.
I'm considering whether `CrossRef` should be dropped in favor of more
specific `Builtin` special-casing, due to the divergence of desired
behaviors. This could mean dropping the `builtins` IR since it's not
looking useful right now.
Modify `NameScope` to track whether the scope is associated with a load
error. This is to handle cases where one or more imports failed, so we
do not want to issue warnings for related scopes.
The 0-size on `ValueStore` comes up due to the changes to `NameScope`,
which make it too large for the default handling. After discussion with
zygoloid, the thought was we might want to try reserving a roughly
correct value based on parse node counts, but the stack default wasn't
buying much.
Fixes a bug where the implicit import used the package name instead of
the invalid identifier.
There's a trade-off here of explicitness in the use versus repetition,
but I'm hoping the Id offers sufficient info (also, some Ids already
relied on this, so this builds consistency). The forward declarations
I'm mixed on, but they are difficult to avoid if heading down this route
due to interdependencies between ids and types which contain ids.
This reflects how we're naming classes that derive from these classes,
and matches usage for each existing `Id` and `Index` type, except:
- `Parse::NodeId` previously inherited from `ComparableIndexBase`, and
is no longer comparable.
- `SemIR::MemberIndex` previously inherited from `IndexBase`, and is now
comparable.
Making `Parse::NodeId` non-comparable reflects that it's intended to be
an opaque identifier for a node and that the ordering is an
implementation detail rather than part of the intended public interface.
`PostorderIterator` and `SiblingIterator` still rely on the numerical
meaning of `NodeId`s, but that's OK since they're part of the node
implementation.
Chandler Carruth
Geoff Romer
cui
Dana Jansens
David Blaikie
Richard Smith
Jon Ross-Perkins
Dana Jansens
Pablo Paglilla