Iterate over arrays by producing their elements in the obvious way. We
use `i32` as the cursor type because that's the type that check converts
array indexes to. This may need revisiting if we support arrays with
more than 2Bi elements.
Also includes a fix for an import crash bug that's triggered by this
change, borrowed from #5873.
Add a new type, `custom_layout_type`, representing a struct type whose
size, alignment, and field offsets can be manually controlled. Use this
as the object representation type for imported C++ class types (which
also includes struct and union types), allowing us to model C++ class
type layouts. In passing, also add support for incomplete C++ class
types, mapping them into incomplete Carbon class types.
Map C++ fields into Carbon field declarations, allowing direct access to
C++ fields from Carbon. So far, no support is added for base classes nor
anonymous struct or union declarations; those will be added in
subsequent PRs. Also, we don't map C++ access control into Carbon yet,
so all C++ fields are accessible regardless of their access control.
For now we still use a `struct_type` as the object representation for
empty C++ classes, in order to continue to support our existing tests
that convert `{}` to empty C++ class types. This is temporary and should
be removed once we support interop with C++ class initialization.
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.
Ensure `vtable_ptr`s(and the vtables they refer to) aren't
imported if the type is imported but the vtable isn't
needed (no initialization of a value of that type is required).
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 preserves the constant values of the arguments to the thunk, which
is important if the thunk requires conversion of an `IntLiteral` to some
other type. This should become unnecessary once we have form support,
but avoiding the indirection through a thunk function seems valuable
even once that support is in place.
To support this, track whether a function is a thunk on the Function
object, and if so, what the callee of the thunk is. This information is
also included in formatted SemIR when dumping the thunk.
Use it to replace most existing modernize-loop-convert lints with
range-based for loops. As requested in review of #5475.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I've been mulling the name of this, changing it and updating comments to
try and better reflect the current semantic. "Imports" reflects how
we're currently printing this in SemIR.
Similar to the class change made in
c6f25e9018 but without tests as it doesn't
appear that the same bug is reachable for these cases at the moment -
but seems good to match the approach in case cycles appear in the future
and to make the code consistent.
The impl case didn't seem to be able to use the new common utility
function, since it splits the two pieces of work and only does the
second conditionally.
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.
This would've identified c6f25e9018
earlier/more clearly.
I looked for similar assertions for things like
`GetLocalConstantValueOrPush` but it has the right property by
construction (if it's going to return `None`, it pushes work) so an
assertion didn't seem suitable there.
Perhaps there are other such mapping functions that could get this
treatment? Open to pointers.
Previously we walked the global variables defined by the current file
and emitted an LLVM global variable definition for each of them. Now
instead, when emitting a constant reference to a global variable, we
emit an LLVM global variable declaration, and we then subsequently walk
the global variables defined by the current file and convert each of
them from a declaration to a definition.
In order to make import of names of global variables work, add support
for import of `var`, as well as support for importing `tuple_access` and
`tuple_pattern` in the case where the `var` has a tuple pattern in its
declaration. Also treat `bind_name`s that are reference bindings to
`var`s as having the same constant reference value as their `var` so
that we can properly import and lower them.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Entities, such as `Function`s may be created during phase 2 and need to
read the `Class`'s `scope_id` at that point, so it must be made
available earlier (in phase 2, rather than 3) when importing.
(thanks @zygoloid for explaining this all to me)
I'll look into other instances of this 3 phase lookup to see if they
have
similar bugs/if I can create test cases to tickle them as follow-ups.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This restructures the import and merge logic to support parameter
patterns in a more scalable way.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
We were importing all impls as non-final, since we forgot to set the new
field when constructing the imported Impl. Adds a test that fails before
this PR, since the imported Impl is treated as non-final.
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>
The canonical location of the instruction may be an entirely different
instruction, which the instruction in question was not imported from. In
particular, we shouldn't assume that we can use the constant value of an
instruction that the *location* of an imported instruction refers to as
the constant value of the imported instruction.
The only time we should be looking at the `ImportIRInstId` for a `LocId`
is when determining its location in some other file.
Fixes a crash when importing thunks (which can contain instructions
whose location points to an instruction in a differnt IR).
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Instead of building an eval block as a separate pass at the end of a
generic, build the eval block incrementally.
The larger change here is that asking for the type or constant value of
an instruction now always returns an unattached type or constant value,
in order to preserve the behavior that we previously achieved by doing
the rewrite to attached types and constant values at the end of handling
the generic.
This also incidentally fixes some subtle issues where attached types and
constant values would leak out into check and cause it to get confused
about differences between attached and unattached values. Check should
no longer see attached values except where it explicitly asks for them.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Don't import `ImplWitnessTable` into the `constants` block, because we
generally don't put `Unique` constants there. This matches the handling
of the other kinds of `Unique` constants. In order to keep the
instruction visible in formatted SemIR, add it to the `imports` block
instead.
Also fix a bug in the instruction formatter that resulted in
instructions in the `imports` block being omitted from the output if
they were only referenced by earlier instructions in the `imports` block
and by instructions in the `constants` block. This was already resulting
in some referenced instructions being omitted from the output, but also
occurred frequently for `impl_witness_table` instructions after this
change because it is common for the only reference to those instructions
to be from `impl_witness` instructions in the `constants` block.
Remove calls to `InstStore::GetLocId()` to build a LocId from an InstId
now that they can be constructed directly from the InstId. Most uses of
LocId are just plumbing, so this does not affect them. However places
that want to look inside the LocId do not want to work with the InstId
form. In these places, introduce `InstStore::GetResolvedLocId()` which
converts a LocId (or an InstId as an optimization) into a LocId which is
not backed by an InstId. These locations can be printed (they have a
line and column when they are a NodeId), they can have flags added to
them (`ToImplicit`, `ToTokenOnly`), they can be converted to an
underlying ImportIRInstId, or they may be `None`.
`Dump()` is made to print a resolved location instead of printing the
InstId in the location, since (at least in my experience) the resolved
location is what is interesting in debugging, and this saves manual
`MakeInstId` steps in the debugger every time a location is of interest.
The LocId constructor from InstId is made `explicit` to add clarity to
function calls passing an `inst_id` now directly instead of calling
`context.insts().GetLocId(inst_id)`. To avoid needing to construct
`SemIR::LocId(...)` explicitly in all cases though, the diagnostics code
in Check uses `DiagnosticLocId` as its template parameter which accepts
InstId as well and does the construction of LocId from it.
Because LocId now requires an explicit construction from InstId, any
callers to `AddInst()` functions will have to explicitly convert to
LocId if they had an InstId, but not if they pass a NodeId. To make this
difference clear to callers, we `requires` that the input type can be
converted to LocId. This ensures that passing an InstId results in an
error at the callsite where the InstId is passed, instead of generating
a compiler error when trying to construct `LocIdAndInst` inside
`AddInst()`, which is less clear about what went wrong and doesn't seem
entirely intentional.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
We eliminate the `FacetAccessWitness` instruction, which would sometimes
immediately evaluate to a concrete `ImplWitness`, and sometimes remain
symbolic. This instruction is now replaced by `LookupImplWitness` in all
cases. To support the same use cases, when it is evaluated,
`LookupImplWitness` will look in the self value if it's a facet value,
and attempt to return a concrete `ImplWitness` from it before looking
for an `impl` statement.
The `LookupImplWitness` instruction's value is now canonical, even when
it evaluates to a symbolic `LookupImplWitness` instruction, by
canonicalizing the self value of the lookup query. This canonicalization
unwraps `FacetAccessType` and `FacetValue` instructions to get to an
underlying canonical facet value. However we must preserve and use the
non-canonical query while evaluating the instruction in order to look
for a concrete `ImplWitness` if the query self value was a concrete
`FacetValue`. The canonicalization ensures that symbolic witnesses
obtained from a facet value are compatible with those obtained from an
impl statement, as long as the self types originate from the same
canonical facet value though they may have been narrowed.
Member access now unconditionally does a `LookupImplWitness()`
operation, instead of only sometimes doing the lookup for a final impl
declaration.
`EvalImplLookupResult` is marked `[[nodiscard]]` so that we don't
construct it and forget to return it. This was a mistake made at one
point during the creation of this PR. And the `has_concrete_value()`
method no longer has a precondition that `has_value()` is true, since we
want to look for a concrete result only in the new use of
`EvalImplLookupResult` returned from lookup into the query self facet
value.
The TODO from `FacetAccessWitness` evaluation is addressed by ensuring
the index of the witness in the `FacetValue` comes from the required
interfaces of the `FacetValue`'s type, and that the type (a `FacetType`)
is the same facet type used in the query to construct the `FacetValue`'s
witness block. This is made possible by eliminating the
`FacetAccessWitness` indirection. The lookup into a `FacetValue` happens
while evaluating `LookupImplWitness` and it does so directly on the self
value. This gives a consistent view of the witness set and the facet
type, as they both come from the same instruction.
All of this with 400 less lines of code. :)
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
These constant instructions are all TypeInstId already in their type,
and this makes their names match.
Change the name of MakeSingletonInstId as well and update its comment.
`Add*InstInNoBlock` adds an instruction in the current context,
including adding its type and constant to the current generic eval block
if necessary. This is inappropriate during import, because the current
generic is generally not related to the instructions we're importing.
Previously we worked around this by pushing a placeholder generic onto
the generics stack, but that workaround doesn't interact well with
building generics incrementally. Instead, change the import code to
create instructions directly instead of via `Add*InstInNoBlock`.
This also allows a little simplification, because all the import logic
created imported instruction locations in the same way.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
We frequently want to operate on singletons. Per discussion, drop
`Singleton` to make the code shorter.
This started off as wanting to write `inst_id.is_error()`, but the
dependency relationship between ids.h and singleton_insts.h would
require some kind of delayed evaluation to allow the implementation to
remain in headers (which I suspect is helpful to have for inlining). I
could have added something like `IsErrorInst`, forward declared in ids.h
and defined in singleton_insts.h (which would always be included by
typed_insts.h), but the template approach felt like a decent balance
between (a) removing the boilerplate `::SingletonInstId`, (b)
understandability, (c) still visually mirroring if we immediately return
a singleton, and (d) flexibility for more than just `ErrorInst`. But TBH
I'd probably still have written `is_error()` if it didn't require
addressing the cross-header cycle.
Then I tried `SemIR::InstId::Is<SemIR::ErrorInst>`, which generally
worked with types but generated the complaint that it didn't shorten
*all* singleton uses. So pulling back on `::Is`, and instead just
dropping `Singleton`.
Use TypeInstId in many more places where the instruction is required
to/known to always be a type value. This should be a somewhat exhaustive
set of places, as it covers all instructions given to
GetTypeIdFromTypeInstId().
The things of interest here are:
- Singleton instructions are always of type TypeType, so they are now
TypeInstIds.
- ErrorInst::SingletonInstId gets upcast to be an InstId because it's
sometimes used to define the type of a variable (as in `auto inst_id =
SemIR::ErrorInst::SingletonInstId;` that may hold other InstIds.
- Parse nodes don't really know about TypeInstId, so NodeStack::Push
needs to do some special casing to avoid CHECK failures when given a
TypeInstId but expecting an InstId. We leave a TODO behind here because
the nodes which are being pushed a TypeInstId should probably be taught
to expect that, but such a change is a bit tricky, so too much for this
PR.
This allows us to import the table for a given impl only once, while we
can import many ImplWitness instructions with different specifics for a
generic impl.
For example in convert_facet_value_to_narrowed_facet_type.carbon we see
that a single witness table is imported for the BitAnd interface, with
multiple witnesses (for different specifics) imported and sharing the
same table.
The ImplWitnessTable now contains a back-link to the Impl the witness is
for, allowing inst namer to name that interface in the textual semir,
and allowing the interface to be found when debugging from a witness.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
After #5280 there are a few more typed instructions that have an `InstId
type_inst_id` that always holds a type value. These are converted to
`TypeInstId` to encode this fact in the type system. The
`ConvertAggregateElement()` function in convert.cpp is now able to
receive `TypeInstId` for a couple arguments as well.
Additionally, the `type_inst_id` field of `StructTypeField` is made into
a `TypeInstId`.
The `TupleType::elements_id` is renamed to `TupleType::type_elements_id`
to try record the fact that it's an InstBlock of type value
instructions. We don't introduce a TypeInstBlockId at this time, but it
might be nice to make blocks of TypeInstIds in the future.
To assist in working with a block of InstId that are type values, two
additional helpers are added to the TypeStore:
- GetBlockAsTypeInstIds which turns an `ArrayRef<InstId>` into a range
of `TypeInstId`
- GetBlockAsTypeIds which turns an `ArrayRef<InstId>` into a range of
`TypeId`
We use these helpers in places that iterate over the
`TupleType::type_elements_id`.
The main goal of this is to collapse the LocId and SemIRLoc types into a
single type, eliminating the need for APIs to decide which to use. This
originated from discussion about UnwrapSemIRLoc in #5169. Although that
was removed in #5202, it's probably still a good direction for LocId.
This changes the packing of LocId to allow adding InstId, making it
tri-modal: ImportIRInstId, InstId, or NodeId. This has a side-effect of
reducing the available space for ImportIRInstId, although not by much
due to the pre-existing `ImplicitBit` behavior. If needed, we could also
probably play with packing a bit more since `ImplicitBit` really only
applies to `NodeId`, but I was trying to keep the logic a little
simpler. Note `TokenOnlyBit` can still apply to `ImportIRInstId`.
This leaves in place a typedef for SemIRLoc -- I intend to clean that up
separately.
Some Discord discussion is
[here](https://discord.com/channels/655572317891461132/655578254970716160/1353755830058745959).
TypeInstId is an InstId whose constant value has a type of TypeType.
This includes:
- Type value instructions, the `ClassType` or `IntLiteralType`
instructions.
- Constraint value instructions, which are the `FacetType` and
`TypeType` instructions, each of which also have type TypeType.
TypeInstId encodes in the type system that it is safe to convert the
instruction's value to a TypeId, and CHECKs at construction that this
invariant is maintained.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
In preparation for shifting from `TypeId`s potentially representing
attached types to always representing unattached types, using
[terminology suggested on
Discord](https://discord.com/channels/655572317891461132/963846118964350976/1359286326779973712).
This change causes us to track slightly more type spelling information
through SemIR.
One change that has significant impact on the SemIR output is that we
now build a `struct_type` instruction in each class representing the
types of the fields, including the spelling used for those types. This
is now no longer always identical to the corresponding canonical
`struct_type` for the object representation, so it's built separately
and owned by the class.
Also remove `TypeBlock` support entirely, as its only use was
representing `TupleType`s, which now use an `InstBlock`.
This follows the pattern used elsewhere, and allows facet types in eval
blocks to directly reference their operands instead of doing so
indirectly via a `ConstantId` attached to the generic. This prepares us
for making `ConstantId`s always be unattached.
In passing, add a stringified version of the `InstId` to diagnostics in
a couple of places where it seems useful.
This gives a slightly simpler representation for `UnboundElementType`s
in eval blocks, and in principle allows us to preserve the spelling of a
field's type into the `UnboundElementType` and thereby into a field
reference, although as of right now this doesn't affect our diagnostic
output in any way.
During error recovery for a field with a non-concrete type, preserve the
type in the `UnboundElementType` regardless. It's not really problematic
to have a non-concrete type there, and this makes it easier to track the
instruction used to specify the type.
This is a step towards switching symbolic types to always be abstract
during type checking.
Instead of storing a `TypeId` that always refer to a facet type that
always contains exactly a single interface, store the interface
directly.
Also improve stringification of `LookupImplWitness` and witness access
into it, switching to using newly-added functionality for stringifying
specific interfaces.
Each of these types takes another type as an operand. Instead of storing
that other type as a `TypeId`, store it as an `InstId` so that we can
track how it was written, not only its canonical form.
The canonical constant values of these types continue to store the
canonical constant values of their operands, as normal.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Trust that import_ref produces constants that are already in their
evaluated form. We still do one pass over the operands to map them into
their canonical constant values. Even that is mostly unnecessary, but
there are a few instructions produced by importing that still need it
for now.
The instruction does act somewhat like a witness, saying that an impl
does exist for a lookup, but the instruction more concretely represents
an impl lookup - since that is done when it is evaluated.
Richard Smith
Jon Ross-Perkins
David BlaikieandDana Jansens
Geoff RomerandRichard Smith
Boaz Brickner
Thomas Köppe
