Replace value_kind with has_type, make FormatInstLhs name-dependent (#5501)

InstValueKind is really just wrapping HasTypeIdMember. Rather than
exposing this as an enum, expose it as a bool since it better reflects
what's going on.

In eval.cpp, AddImportedConstant should never be called on an untyped
instruction.

In FormatInstLhs, we can also depend on whether InstNamer has assigned a
name in order to decide whether to print an instruction. This should
avoid some divergence with CollectNamesInBlock.

We also discussed restoring InstValueKind::Untyped, but that's mainly
motivated by the formatter, and the InstNamer approach gives a more
localized implementation.

toolchain/check/eval.cpp

@@ -772,22 +772,14 @@ static auto ReplaceTypeWithConstantValue(EvalContext& eval_context,
 auto AddImportedConstant(Context& context, SemIR::Inst inst)
     -> SemIR::ConstantId {
   EvalContext eval_context(&context, SemIR::LocId::None);
-  Phase phase = Phase::Concrete;
-  switch (inst.kind().value_kind()) {
-    case SemIR::InstValueKind::Typed: {
-      phase = GetPhase(context.constant_values(),
-                       context.types().GetConstantId(inst.type_id()));
-      // TODO: Can we avoid doing this replacement? It may do things that are
-      // undesirable during importing, such as resolving specifics.
-      if (!ReplaceAllFieldsWithConstantValues(eval_context, &inst, &phase)) {
-        return SemIR::ConstantId::NotConstant;
-      }
-      break;
-    }
-    case SemIR::InstValueKind::None: {
-      // Instructions without a type_id are not evaluated.
-      break;
-    }
+  CARBON_CHECK(inst.kind().has_type(), "Can't import untyped instructions: {0}",
+               inst.kind());
+  Phase phase = GetPhase(context.constant_values(),
+                         context.types().GetConstantId(inst.type_id()));
+  // TODO: Can we avoid doing this replacement? It may do things that are
+  // undesirable during importing, such as resolving specifics.
+  if (!ReplaceAllFieldsWithConstantValues(eval_context, &inst, &phase)) {
+    return SemIR::ConstantId::NotConstant;
   }
   return MakeConstantResult(context, inst, phase);
 }

toolchain/docs/adding_features.md

@@ -355,7 +355,7 @@ How does this work? As of
 
 -   [sem_ir/inst_kind.h](/toolchain/sem_ir/inst_kind.h) includes
     [common/enum_base.h](/common/enum_base.h) and defines an enumeration
-    `InstKind`, along with `InstValueKind` and `TerminatorKind`.
+    `InstKind`, along with `TerminatorKind`.
 
     -   The `InstKind` enumeration is populated with the list of all instruction
         kinds using [sem_ir/inst_kind.def](/toolchain/sem_ir/inst_kind.def)
@@ -370,7 +370,7 @@ How does this work? As of
         with the same enumerant value, plus values for the other fields.
 
 -   Note that additional information is needed to define the `ir_name()`,
-    `value_kind()`, and `terminator_kind()` methods of `InstKind`. This
+    `has_type()`, and `terminator_kind()` methods of `InstKind`. This
     information comes from the typed instruction definitions in
     [sem_ir/typed_insts.h](/toolchain/sem_ir/typed_insts.h).
 
@@ -399,9 +399,8 @@ How does this work? As of
         kinds from [sem_ir/inst_kind.def](/toolchain/sem_ir/inst_kind.def)
         (using [the .def file idiom](idioms.md#def-files))
 
-    -   `InstKind::value_kind()` is defined. It has a static table of
-        `InstValueKind` values indexed by the enum value, populated by applying
-        `HasTypeIdMember` from
+    -   `InstKind::has_type()` is defined. It has a static table of indexed by
+        the enum value, populated by applying `HasTypeIdMember` from
         [sem_ir/typed_insts.h](/toolchain/sem_ir/typed_insts.h) to every
         instruction kind by using the list from
         [sem_ir/inst_kind.def](/toolchain/sem_ir/inst_kind.def).

toolchain/sem_ir/formatter.cpp

@@ -829,43 +829,37 @@ auto Formatter::FormatPendingConstantValue(AddSpace space_where) -> void {
 }
 
 auto Formatter::FormatInstLhs(InstId inst_id, Inst inst) -> void {
-  switch (inst.kind()) {
-    case InstKind::ImplWitnessTable:
-    case InstKind::ImportCppDecl:
-    case InstKind::ImportDecl:
-    case InstKind::ImportRefUnloaded:
-      // Although these don't have a typed value, we still want to print the
-      // name.
-      FormatName(inst_id);
-      out_ << " = ";
-      return;
-
-    default:
-      switch (inst.kind().value_kind()) {
-        case InstValueKind::Typed:
-          FormatName(inst_id);
-          out_ << ": ";
-          switch (GetExprCategory(*sem_ir_, inst_id)) {
-            case ExprCategory::NotExpr:
-            case ExprCategory::Error:
-            case ExprCategory::Value:
-            case ExprCategory::Mixed:
-              break;
-            case ExprCategory::DurableRef:
-            case ExprCategory::EphemeralRef:
-              out_ << "ref ";
-              break;
-            case ExprCategory::Initializing:
-              out_ << "init ";
-              break;
-          }
-          FormatTypeOfInst(inst_id);
-          out_ << " = ";
-          break;
-        case InstValueKind::None:
-          break;
-      }
+  // Every typed instruction is named, and there are some untyped instructions
+  // that have names (such as `ImportRefUnloaded`).
+  bool has_name = inst_namer_.has_name(inst_id);
+  if (!has_name) {
+    CARBON_CHECK(!inst.kind().has_type(),
+                 "Missing name for typed instruction: {0}", inst);
+    return;
   }
+
+  FormatName(inst_id);
+
+  if (inst.kind().has_type()) {
+    out_ << ": ";
+    switch (GetExprCategory(*sem_ir_, inst_id)) {
+      case ExprCategory::NotExpr:
+      case ExprCategory::Error:
+      case ExprCategory::Value:
+      case ExprCategory::Mixed:
+        break;
+      case ExprCategory::DurableRef:
+      case ExprCategory::EphemeralRef:
+        out_ << "ref ";
+        break;
+      case ExprCategory::Initializing:
+        out_ << "init ";
+        break;
+    }
+    FormatTypeOfInst(inst_id);
+  }
+
+  out_ << " = ";
 }
 
 auto Formatter::FormatInstRhs(BindSymbolicName inst) -> void {

toolchain/sem_ir/formatter.h

@@ -230,6 +230,8 @@ class Formatter {
   // no such arguments.
   auto FormatPendingConstantValue(AddSpace space_where) -> void;
 
+  // Formats `<name>[: <type>] = `. Skips unnamed instructions (according to
+  // `inst_namer_`). Typed instructions must be named.
   auto FormatInstLhs(InstId inst_id, Inst inst) -> void;
 
   template <typename InstT>

toolchain/sem_ir/inst_kind.cpp

@@ -21,11 +21,9 @@ auto InstKind::definition_info(InstKind inst_kind) -> const DefinitionInfo& {
   return DefinitionInfos[inst_kind.AsInt()];
 }
 
-auto InstKind::value_kind() const -> InstValueKind {
-  static constexpr InstValueKind Table[] = {
-#define CARBON_SEM_IR_INST_KIND(Name)                           \
-  Internal::HasTypeIdMember<SemIR::Name> ? InstValueKind::Typed \
-                                         : InstValueKind::None,
+auto InstKind::has_type() const -> bool {
+  static constexpr bool Table[] = {
+#define CARBON_SEM_IR_INST_KIND(Name) Internal::HasTypeIdMember<SemIR::Name>,
 #include "toolchain/sem_ir/inst_kind.def"
   };
   return Table[AsInt()];

toolchain/sem_ir/inst_kind.h

@@ -21,17 +21,6 @@ enum class InstIsType : int8_t {
   Never,
 };
 
-// Whether an instruction produces or represents a value, and if so, what kind
-// of value.
-enum class InstValueKind : int8_t {
-  // This instruction doesn't produce a value, and shouldn't be referenced by
-  // other instructions.
-  None,
-  // This instruction represents an expression or expression-like construct that
-  // produces a value of the type indicated by its `type_id` field.
-  Typed,
-};
-
 // Whether an instruction can have a constant value, and whether it can be used
 // to define a constant value.
 //
@@ -169,8 +158,8 @@ class InstKind : public CARBON_ENUM_BASE(InstKind) {
   // Returns whether this instruction kind defines a type.
   auto is_type() const -> InstIsType { return definition_info(*this).is_type; }
 
-  // Returns whether this instruction kind is expected to produce a value.
-  auto value_kind() const -> InstValueKind;
+  // Returns whether this instruction kind is expected to produce a typed value.
+  auto has_type() const -> bool;
 
   // Returns this instruction kind's category of allowed constants.
   auto constant_kind() const -> InstConstantKind {

toolchain/sem_ir/inst_namer.cpp

@@ -569,7 +569,8 @@ auto InstNamer::CollectNamesInBlock(ScopeId top_scope_id,
       case CARBON_KIND(Call inst): {
         auto callee_function = GetCalleeFunction(*sem_ir_, inst.callee_id);
         if (!callee_function.function_id.has_value()) {
-          break;
+          add_inst_name("");
+          continue;
         }
         const auto& function =
             sem_ir_->functions().Get(callee_function.function_id);
@@ -595,9 +596,9 @@ auto InstNamer::CollectNamesInBlock(ScopeId top_scope_id,
         if (auto literal_info = NumericTypeLiteralInfo::ForType(*sem_ir_, inst);
             literal_info.is_valid()) {
           add_inst_name(literal_info.GetLiteralAsString(*sem_ir_));
-          break;
+        } else {
+          add_inst_name_id(sem_ir_->classes().Get(inst.class_id).name_id);
         }
-        add_inst_name_id(sem_ir_->classes().Get(inst.class_id).name_id);
         continue;
       }
       case CompleteTypeWitness::Kind: {
@@ -685,7 +686,7 @@ auto InstNamer::CollectNamesInBlock(ScopeId top_scope_id,
       case CARBON_KIND(ImplDecl inst): {
         auto impl_scope_id = GetScopeFor(inst.impl_id);
         queue_block_id(impl_scope_id, inst.decl_block_id);
-        break;
+        continue;
       }
       case CARBON_KIND(LookupImplWitness inst): {
         const auto& interface = sem_ir_->specific_interfaces().Get(
@@ -859,11 +860,12 @@ auto InstNamer::CollectNamesInBlock(ScopeId top_scope_id,
       }
       case ReturnSlot::Kind: {
         add_inst_name_id(NameId::ReturnSlot);
-        break;
+        continue;
       }
       case CARBON_KIND(SpliceBlock inst): {
         queue_block_id(scope_id, inst.block_id);
-        break;
+        add_inst_name("");
+        continue;
       }
       case StringLiteral::Kind: {
         add_inst_name("str");
@@ -953,14 +955,13 @@ auto InstNamer::CollectNamesInBlock(ScopeId top_scope_id,
         continue;
       }
       default: {
-        break;
+        // Sequentially number all remaining values.
+        if (untyped_inst.kind().has_type()) {
+          add_inst_name("");
+        }
+        continue;
       }
     }
-
-    // Sequentially number all remaining values.
-    if (untyped_inst.kind().value_kind() != InstValueKind::None) {
-      add_inst_name("");
-    }
   }
 }
 

toolchain/sem_ir/inst_namer.h

@@ -98,6 +98,11 @@ class InstNamer {
   // Returns the IR name to use for a label, when referenced from a given scope.
   auto GetLabelFor(ScopeId scope_id, InstBlockId block_id) const -> std::string;
 
+  // Returns true if the instruction has a specific name assigned.
+  auto has_name(InstId inst_id) const -> bool {
+    return static_cast<bool>(insts_[inst_id.index].second);
+  }
+
  private:
   // A space in which unique names can be allocated.
   struct Namespace {