Trying to make it easier to see the API at a glance. The class has
become really long, and this doesn't fundamentally change that, but
hopefully makes it easier to navigate. The entry structure also had some
cruft that I'm removing.
I'm trying to keep functions in the same order as they currently are.
The delta still looks unhappy because of the churn, but hopefully this
at least explains the ordering in formatter.h. You can try using the
"Add indent" commit on the PR to see a better before-after delta.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
The location of the diagnostic is an instruction id, not a parse node.
Converting to a parse node to call ToTokenOnly will drop the descendents
of the instruction being diagnosed, but the diagnostic is about the
whole instruction not just the root parse node of whatever instruction
it happens to be.
This was originally needed to support constant evaluation of name
expressions, but that's now done in a different way.
This is actually a step toward treating all patterns as constants. The
upcoming change will do so in a slightly different way, and so it will
simplify the review to start from a baseline where patterns are never
constant.
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>
Trying to make it easier to skim test output for failures. Note this
excludes all the RUN and OK. Failing tests may still be verbose due to
the diff printed, but CHECK-fails should become short.
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 128 thread(s)
...
Done!
[==========] 1272 tests from 1 test suite ran. (688 ms total)
[ PASSED ] 1272 tests.
================================================================================
```
Trying to make it easier to see possible bottlenecks.
Disabling hyperthreading seems like a significant reduction in
contention (15% improvement for me). Going down by half again reduces a
contention a little further, but not significantly from what I see. My
thought is that just flipping the flag is going to work best for people
cross-system versus a "divide by four", but welcome to other opinions
there. Note, I'm not digging into the source of the contention here,
just observing it.
Current default on my system (equivalent to `--threads=128`):
```
Running tests with 128 thread(s)
...
Ran 1272 tests in 3955 ms wall time, 397615 ms across threads
```
Disabling hyperthreads (equivalent to `--threads=64`):
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3520 ms wall time, 161957 ms across threads
```
`--threads=32`:
```
Running tests with 32 thread(s)
...
Ran 1272 tests in 3329 ms wall time, 69327 ms across threads
```
And for `./autoupdate_testdata.py --threads=64 --print_slowest_tests=5`:
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3417 ms wall time, 157946 ms across threads
Slowest tests:
- toolchain/lower/testdata/function/generic/call_recursive_basic.carbon: 1508 ms, 1484 ms in Run
- toolchain/lower/testdata/builtins/print_read.carbon: 1506 ms, 1506 ms in Run
- toolchain/lower/testdata/array/field.carbon: 1488 ms, 1487 ms in Run
- toolchain/lower/testdata/builtins/int.carbon: 1482 ms, 1475 ms in Run
- toolchain/lower/testdata/function/definition/params_one.carbon: 1472 ms, 1471 ms in Run
```
In test:
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 64 thread(s)
...
Ran 1272 tests in 2968 ms wall time, 177732 ms across threads
Slowest tests:
- toolchain/lower/testdata/builtins/int.carbon: 1544 ms, 1533 ms in Run
- toolchain/lower/testdata/array/function_param.carbon: 1539 ms, 1537 ms in Run
- toolchain/lower/testdata/basics/zero.carbon: 1537 ms, 1536 ms in Run
- toolchain/lower/testdata/function/call/params_one.carbon: 1535 ms, 1534 ms in Run
- toolchain/lower/testdata/function/definition/params_zero.carbon: 1531 ms, 1531 ms in Run
[==========] Running 1272 tests from 1 test suite.
[----------] Global test environment set-up.
```
Not entirely sure what the SemIR representation for this should be - do
we put the bogus thing in the vtable, and just not lower it later? The
patch currently doesn't add the function to the SemIR vtable - which
then means you could find a virtual function that's not in the vtable,
which seems similarly confusing.
I guess we could make the function non-virtual?
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Syntax rationale is on `DumpSemIRRange` to try and record this, since
I'm not sure this belongs in the language design. The intent of this is
to be able to subset SemIR, which will be done separately in the
formatter.
---------
Co-authored-by: David Blaikie <dblaikie@gmail.com>
These will need to be emitted lazily, as we do for functions - this
addresses the crash/removes the impossible (because we don't have a
specific) non-lazy path.
Adds an empty `min_prelude/destroy.carbon` in anticipation of turning it
into an interface. Update `no_prelude` tests to be `min_prelude` and
import it where needed; in some cases, modify the file to remove the
dependency (i.e., rewrite code to have nothing to destruct).
Right now, return_scope_stack is being used to determine whether logic
is in a function scope. However, we need to handle nested entities
inside function scopes. For example where this crashes right now:
```
base class C(B:! bool) {}
fn F() {
class B {
extend base: C(true or false);
}
}
```
This is doing a few things to make this kind of code not crash:
- Split `scope_stack().Push` into `PushForDeclName`, `PushForEntity`,
`PushForExpr`, and `PushForFunction` so that better decisions can be
made about behaviors.
- Hide `return_scope_stack` in the API, instead using interfaces to get
at the underlying data.
- Also using `PushForFunction` to update it similar to the other stacks
that `ScopeStack` manages.
- Add `IsInFunctionScope` as the best way to determine presence in
function scope.
- Remove `PeekIsLexicalScope` since destruction really wants function
scope information anyways.
- Clean up `destroy_id_stack` handling to be for function scopes rather
than lexical scopes.
- Return after related `context.TODO`s in a couple more spots, so that
code doesn't proceed to add control flow in spite of the lack of
support.
---------
Co-authored-by: Geoff Romer <gromer@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>
In a class, an `impl as` can now be both `final` and `extend` instead of
only one or the other.
In https://github.com/carbon-language/carbon-lang/issues/5319 we decided
this is already allowed by the design but was an oversight in the
implementation.
A `MetaInstId` is intended to represent a handle to an instruction in a
generic as an operand to a template action; substituting into the action
should not substitute into the referenced instruction.
Fixing this exposed a bug in `GetOrAddInst` where it would return an
`InstId` of an unattached symbolic constant in some cases, rather than
the `InstId` of an instruction that has the relevant (attached) constant
value. That's fixed for now by turning off the `GetOrAddInst`
optimization in that case, but in future we can refine this by adding
the instruction to the eval block for the generic only, and not to the
body of the generic.
Provide builtins for compound assignments instead of defining them in
the prelude as a use of a binary operator and an assignment. This allows
us to lower compound assignment directly to LLVM operations instead of
producing a function call. In the short term this also allows us to
define a type-generic compound assignment in the prelude.
Instead of evaluating a non-parameterized class or interface to a
constant with `SpecificId::None`, use the self specific for that class
or interface, which will not be `None` if there is an enclosing generic.
`FindIfOrNull` returns a pointer to the element in the range if it's
found, and nullptr otherwise. `FindIfOrNone` returns a copy of the
element in the range if it's found, and `T::None` (for a range of
elements of type `T`) otherwise. `Contains` returns a bool indicating
whether the element in the range is found.
These functions replace `llvm::find()` and `llvm::find_if()` when you
want a single answer back instead of an iterator. This avoids the need
to check against `end()`, allowing the return condition to be tested as
a standard bool.
We replace uses of `find()` and `find_if()` that did not require an
iterator with these new helpers.
Note that the return type of `FindIfOrNull` is a pointer since we can
not write `optional<T&>`, which must be tested for null. If the null
check is omitted, UB occurs and the resulting code may end up with an
incorrect pointer (https://crbug.com/40153300) into the range (or
elsewhere), rather than a null dereference. And this would be very
confusing to debug. Hopefully debug builds and sanitizers keep this from
being an issue we sink a bunch of time into debugging.
This fake generic was used for two reasons:
- The declaration name stack assumes that each declaration name is
processed within a generic scope. This is important if the name might
have generic parameters, which are always parsed even for declarations
that disallow them in check.
- Out-of-line redeclarations of generic entities produce instructions
with symbolic constant values in non-generic scopes.
The former case is addressed by pushing a generic each time we start a
declaration name, even if we will reject generic parameters later. The
latter case is worked around for now by not building a symbolic constant
type or value for instructions that appear outside of any generic, and
will be addressed more completely by #5310 and follow-ups.
The docs explain that you must use `--local-lldbinit` in the command
line, and include an example of how to run a file_test under lldb from
the command line.
This PR includes `.lldbinit` file and `lldbinit.py` file which set up
our default options, copied from the VSCode launcher.
The instructions include settin the `max-string-summary-length`, and we
include this in the vscode launcher for lldb, as printing `Dump()`
output can easily get truncated otherwise when printing an InstBlockId.
We already had conversion in place to implicitly convert these literals
to `type`. Now they can also convert to `FacetType`. This is done by
first doing a conversion to `type` and then converting that type value
to `FacetType`.
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.
Adds a `virtual_index` to `SemIR::Function` used to determine which
vtable slot
to use when calling the given function.
Then use that to lower the function call to use the vtable and
specifically the
relative vtable ABI to match the vtable entries.
`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>
Jon Ross-PerkinsandDana Jansens
Boaz Brickner
Geoff Romer
Ivan Duranandjosh11b
David BlaikieandJon Ross-Perkins
Chandler Carruth
Thomas Köppe