See
[here](https://docs.google.com/document/d/1rWcueFwIfZox6GKVGxiUG4cBzjrZ6djXiIDGyJDtrE4/edit?tab=t.0)
for the design doc.
This also removes the default value of the `result_type_inst_id`
parameter of `HandleAction`, moves it before the action in the parameter
list, and documents it. This solves two problems:
- The default made it easy to forget, leading to unnecessary
`TypeOfInst` instructions.
- When it was present, putting it after the fairly "bulky" action
argument tended to make the callsite harder to read.
Instead, use the inst category to select the right block stack. This
simplifies the API for adding insts, and in subsequent changes it will
enable certain inst kinds like `SpliceInst` to seamlessly function as
either procedural insts or pattern insts.
EvalOrAddInst has to create a non-canonical instruction for evaluating a
few typed insts, such as LookupImplWitness which uses an InstId to
provide a location for diagnostics.
But the output of the function is a ConstantId. We do not have access to
the non-canonical InstId after the function returns. But if the constant
value was symbolic, it was being attached to the inst, and the inst
would be added to the eval block of the enclosing generic. This
needlessly added semir for a symbolic value.
The ConstantId returned by EvalOrAddInst can be used immediately, such
as to evaluate an ImplWitnessAccess. In that case, the final evaluated
result is all we need to keep in semir.
If the ConstantId needs to be replaced by specifics, it is only as part
of some other instruction, since ConstantIds themselves are not modified
by specifics, instructions are. In that case, the canonical instruction
in the constant value would have been added to some other (now symbolic)
instruction, which would be replaced by a specific.
This has no functional change, but it reduces runtime overhead and semir
output for LookupImplWitness and ImplWitnessAccess.
This follows up on a discussion about wanting to use `Any*` inst
clusters to handle boilerplate construction, with the issue that
`UncheckedLoc` use removes validation. Some context is at
https://github.com/carbon-language/carbon-lang/pull/6930#discussion_r2963157428.
This folds in `MakeImportedLocIdAndInst` because the logic is related,
particularly for `LocId` values which are `ImportIRInstId`, and it
eliminates questions of what the right function is to use.
This uncovers an error in the `NodeKind` associated with
`FormBindingPattern`. For now I'm just adding a TODO regarding that.
Assisted-by: Google Antigravity with Gemini
- Makes a little more use of `MakeImportedLocIdAndInst` instead of
`UncheckedLoc`
- Requires use of `MakeImportedLocIdAndInst` with `ImportIRInstId`;
previously optional
- Relevant `if constexpr` moves to `AddPlaceholderImportedInst`, but is
more narrowly scoped there.
- Refactors out `AddPlaceholderImportedInstInNoBlock` to reduce how many
spots do an explicit `imports().push_back(...)`
I'd also considered removing `MakeImportedLocIdAndInst` where possible,
but went this route so that changes to the expected parse node wouldn't
affect callers. When it's required, `MakeImportedLocIdAndInst` is always
there; when it's conditionally present, changing `Parse::NodeId` between
enforceable and not-enforceable would require refactoring any callsites
that assumed one or the other.
This is a bit of an experiment to see if there's a reasonable way to
write a shared enum type, rather than writing per-case wrappers for
things like `HasTypeQualifiers` or the printing. I think it's a bit
borderline complexity right now, but I'm not sure I can reduce it much
further.
This changes from things like `Internal::EnumClassName##RawEnum` to
`Internal::EnumClassName##Data::RawEnum` so that the enum entries can
have back references to bit shifts without needing to know the
containing type name. Because I'm trying to reduce duplication between
mask and non-mask enums, I did this to non-mask enums too.
This was motivated by #6035 adding another enum mask (which will grow
more entries, and is intended to switch if this is accepted), but I'm
not using that PR as a base here because I didn't want the merge
dependency.
This changes `ToImplicit` to `AsDesugared`, and adds a
`GetLocIdForDesugaring` to `InstStore`.
In particular, I'm motivated by the latter, to make it clearer what the
intended call convention is.
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>
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.
Use that instead of `AddInstInNoBlock` to get the value of an
instruction when evaluation might depend on the `InstId` but only the
`ConstantId` of the instruction is desired by the consumer.
For each kind of instruction, specify whether its constant evaluation
needs an `InstId` or not. If it does, ensure that all constant
evaluation of that instruction provides one. Otherwise, allow calling
into the evaluator without providing an `InstId`.
This allows us to reliably use the `InstId` in evaluation steps that
either need a location or need to look at the original operands of the
instruction prior to evaluation, and also to support `TryEvalInst` calls
safely for instructions whose evaluation does not need an `InstId`.
This in particular uses free functions because it's likely to end up
more consistent with types (versus a wrapper object for InstStore).
Note, this is unlikely to have a performance impact, but if it does, we
can look into related approaches (and we've already discussed using
LTO).
Renames `PendingBlock::AddInst` to `PendingBlock::Add` because
`MakeElementAccessInst` expects the matching name to exist.
Geoff Romer
Dana Jansens
Jon Ross-Perkins
Richard SmithandDana Jansens