reviewer feedback

toolchain/docs/check.md

@@ -89,23 +89,18 @@ instruction, as described in
     instructions, which are modeled as producing a value of "namespace" type,
     even though they can't be used as a first-class value in Carbon expressions.
 
--   Up to two additional, kind-specific members. For example `SemIR::Assign` has
-    members `InstId lhs_id` and `InstId rhs_id`.
-
--   Optionally, an `ElementIndex index` field for instructions that refer to a
-    single element from a larger set. For example, `SemIR::ClassElementAccess`
-    has an `ElementIndex` into the fields of the class to define which field it
-    is accessing.
+-   Up to two additional, kind-specific members, which are typically ids or
+    indexes derived from `IdBase`. For example `SemIR::Assign` has members
+    `InstId lhs_id` and `InstId rhs_id`. And `SemIR::ClassElementAccess` has an
+    `ElementIndex` into the fields of the class to define which field it is
+    accessing. See the comment in
+    [sem_ir/typed_insts.h](/toolchain/sem_ir/typed_insts.h) for more details.
 
 Instructions are stored as type-erased `SemIR::Inst` objects, which store the
 instruction kind and the (up to) four fields described above. This balances the
-size of `SemIR::Inst` against the overhead of indirection.
-
-Most instructions have with them a `LocId` that tracks their location, and is
-stored on the `InstStore` and retrieved by `GetLocId()`. The exception to this
-is `SemIR::Builtin` which do not come with an associated location (as they do
-not come from source code) and for which `GetLocId()` will return
-`LocId::Invalid`.
+size of `SemIR::Inst` against the overhead of indirection. All instructions have
+with them a `LocId` that tracks their source location, and is stored separately
+on the `InstStore` and retrieved by `GetLocId()`.
 
 A `SemIR::InstBlock` can represent a code block. However, it can also be created
 when a series of instructions needs to be closely associated, such as a

toolchain/sem_ir/id_kind.h

@@ -115,6 +115,11 @@ class TypeEnum {
 };
 
 // An enum of all the ID types used as instruction operands.
+//
+// As instruction operands, the types listed here can appear as fields of typed
+// instructions (`toolchain/sem_ir/typed_insts.h`) and must implement the
+// `FromRaw` and `ToRaw` protocol in `SemIR::Inst`. In most cases this is done
+// by inheriting from `IdBase` or `IndexBase`.
 using IdKind = TypeEnum<
     // From base/value_store.h.
     IntId, RealId, FloatId, StringLiteralValueId,

toolchain/sem_ir/typed_insts.h

@@ -20,9 +20,10 @@
 // - Optionally, a `TypeId type_id;` member, for instructions that produce a
 //   value. This includes instructions that produce an abstract value, such as a
 //   `Namespace`, for which a placeholder type should be used.
-// - Up to two `[...]Id` members describing the contents of the struct.
-// - Optionally, a `ElementIndex index;` member, for instructions that refer to
-//   one of a set of elements (e.g. a field from a class).
+// - Up to two members describing the contents of the struct. These are types
+//   listed in the `SemIR::IdKind` type-enum, typically derived from `IdBase`.
+//
+// TODO: There are some `int32_t` fields, which breaks the above rule, how/why?
 //
 // The field names here matter -- the fields must have the names specified
 // above, when present. When converting to a `SemIR::Inst`, the `kind` and
@@ -34,16 +35,16 @@
 // enumeration. This `Kind` declaration also defines the instruction kind by
 // calling `InstKind::Define` and specifying additional information about the
 // instruction kind. This information is available through the member functions
-// of the `InstKind` value declared in `inst_kind.h`, and includes the name
-// used in textual IR and whether the instruction is a terminator instruction.
+// of the `InstKind` value declared in `inst_kind.h`, and includes the name used
+// in textual IR and whether the instruction is a terminator instruction.
 //
 // Struct types can also be provided for categories of instructions with a
 // common representation, to allow the common representation to be accessed
 // conveniently. In this case, instead of providing a constant `Kind` member,
 // the struct should have a constant `InstKind Kinds[];` member that lists the
-// kinds of instructions in the category, and an `InstKind kind;` member that
-// is used to identify the specific kind of the instruction. Separate struct
-// types still need to be defined for each instruction kind in the category.
+// kinds of instructions in the category, and an `InstKind kind;` member that is
+// used to identify the specific kind of the instruction. Separate struct types
+// still need to be defined for each instruction kind in the category.
 
 namespace Carbon::SemIR {