Просмотр исходного кода

Include concrete non-type values in the type structure and use for impl candidate selection (#5431)

Non-type values were being represented as "Concrete" in the type
structure's shape, which was incorrect when the value was symbolic. Now
they are represented as "Concrete" or "Symbolic" as appropriate.

When concrete, a matching concrete ConstantId is stored in the type
structure's concrete values, so that they will be used for equality
comparison. And then impl matching comparison is taught to look in the
concrete values for mismatches when it finds a "Concrete" or
"ConcreteOpenParen" shape on both sides of the comparison, and return
false if they are not equal. This reduces the number of candidate impls
selected in impl lookup, which avoids doing type deduction against impls
that won't match anyway due to the concrete values in the query not
matching concrete values in the impl. As a result we see fewer specifics
being generated in the semir.
Dana Jansens 11 месяцев назад
Родитель
Сommit
e6a6624ec6

+ 10 - 43
toolchain/check/testdata/facet/min_prelude/convert_class_value_to_generic_facet_value_value.carbon

@@ -639,12 +639,12 @@ fn B() {
 // CHECK:STDOUT:   %Self.770: %I.type.f76 = bind_symbolic_name Self, 2 [symbolic]
 // CHECK:STDOUT:   %C: type = class_type @C [concrete]
 // CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
-// CHECK:STDOUT:   %complete_type.357: <witness> = complete_type_witness %empty_struct_type [concrete]
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete]
 // CHECK:STDOUT:   %T.8b3: type = bind_symbolic_name T, 0 [symbolic]
 // CHECK:STDOUT:   %I.type.bea: type = facet_type <@I, @I(%T.8b3, %empty_tuple.type)> [symbolic]
 // CHECK:STDOUT:   %Self.ec5: %I.type.bea = bind_symbolic_name Self, 2 [symbolic]
 // CHECK:STDOUT:   %require_complete.5ce: <witness> = require_complete_type %I.type.bea [symbolic]
-// CHECK:STDOUT:   %I.impl_witness.989: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.8b3) [symbolic]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.8b3) [symbolic]
 // CHECK:STDOUT:   %I.type.906: type = facet_type <@I, @I(%empty_struct_type, %empty_struct_type)> [concrete]
 // CHECK:STDOUT:   %T.4b2: %I.type.906 = bind_symbolic_name T, 0 [symbolic]
 // CHECK:STDOUT:   %pattern_type.a62: type = pattern_type %I.type.906 [concrete]
@@ -656,10 +656,6 @@ fn B() {
 // CHECK:STDOUT:   %B.type: type = fn_type @B [concrete]
 // CHECK:STDOUT:   %B: %B.type = struct_value () [concrete]
 // CHECK:STDOUT:   %C.val: %C = struct_value () [concrete]
-// CHECK:STDOUT:   %I.type.202: type = facet_type <@I, @I(%empty_struct_type, %empty_tuple.type)> [concrete]
-// CHECK:STDOUT:   %Self.38c: %I.type.202 = bind_symbolic_name Self, 2 [symbolic]
-// CHECK:STDOUT:   %complete_type.3d9: <witness> = complete_type_witness %I.type.202 [concrete]
-// CHECK:STDOUT:   %I.impl_witness.806: <witness> = impl_witness file.%I.impl_witness_table, @impl(%empty_struct_type) [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {
@@ -697,7 +693,7 @@ fn B() {
 // CHECK:STDOUT:     %T.loc7_14.1: type = bind_symbolic_name T, 0 [symbolic = %T.loc7_14.2 (constants.%T.8b3)]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %I.impl_witness_table = impl_witness_table (), @impl [concrete]
-// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness %I.impl_witness_table, @impl(constants.%T.8b3) [symbolic = @impl.%I.impl_witness (constants.%I.impl_witness.989)]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness %I.impl_witness_table, @impl(constants.%T.8b3) [symbolic = @impl.%I.impl_witness (constants.%I.impl_witness)]
 // CHECK:STDOUT:   %A.decl: %A.type = fn_decl @A [concrete = constants.%A] {
 // CHECK:STDOUT:     %T.patt: %pattern_type.a62 = symbolic_binding_pattern T, 0 [concrete]
 // CHECK:STDOUT:     %t.patt: @A.%pattern_type (%pattern_type.f1c) = binding_pattern t [concrete]
@@ -744,7 +740,7 @@ fn B() {
 // CHECK:STDOUT:   %T.loc7_14.2: type = bind_symbolic_name T, 0 [symbolic = %T.loc7_14.2 (constants.%T.8b3)]
 // CHECK:STDOUT:   %I.type.loc7_36.2: type = facet_type <@I, @I(%T.loc7_14.2, constants.%empty_tuple.type)> [symbolic = %I.type.loc7_36.2 (constants.%I.type.bea)]
 // CHECK:STDOUT:   %require_complete: <witness> = require_complete_type %I.type.loc7_36.2 [symbolic = %require_complete (constants.%require_complete.5ce)]
-// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.loc7_14.2) [symbolic = %I.impl_witness (constants.%I.impl_witness.989)]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.loc7_14.2) [symbolic = %I.impl_witness (constants.%I.impl_witness)]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !definition:
 // CHECK:STDOUT:
@@ -756,7 +752,7 @@ fn B() {
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @C {
 // CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete = constants.%empty_struct_type]
-// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete = constants.%complete_type.357]
+// CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete = constants.%complete_type]
 // CHECK:STDOUT:   complete_type_witness = %complete_type
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
@@ -807,7 +803,7 @@ fn B() {
 // CHECK:STDOUT:   %T.loc7_14.2 => constants.%T.8b3
 // CHECK:STDOUT:   %I.type.loc7_36.2 => constants.%I.type.bea
 // CHECK:STDOUT:   %require_complete => constants.%require_complete.5ce
-// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness.989
+// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: specific @I(constants.%empty_struct_type, constants.%empty_struct_type) {
@@ -821,22 +817,6 @@ fn B() {
 // CHECK:STDOUT:   %pattern_type => constants.%pattern_type.f1c
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: specific @impl(constants.%empty_struct_type) {
-// CHECK:STDOUT:   %T.loc7_14.2 => constants.%empty_struct_type
-// CHECK:STDOUT:   %I.type.loc7_36.2 => constants.%I.type.202
-// CHECK:STDOUT:   %require_complete => constants.%complete_type.3d9
-// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness.806
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: specific @I(constants.%empty_struct_type, constants.%empty_tuple.type) {
-// CHECK:STDOUT:   %V.loc3_13.2 => constants.%empty_struct_type
-// CHECK:STDOUT:   %W.loc3_23.2 => constants.%empty_tuple.type
-// CHECK:STDOUT:
-// CHECK:STDOUT: !definition:
-// CHECK:STDOUT:   %I.type => constants.%I.type.202
-// CHECK:STDOUT:   %Self.2 => constants.%Self.38c
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
 // CHECK:STDOUT: --- fail_mismatch_impl_self_with_fixed_specific.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
@@ -853,7 +833,7 @@ fn B() {
 // CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete]
 // CHECK:STDOUT:   %T.8b3: type = bind_symbolic_name T, 0 [symbolic]
 // CHECK:STDOUT:   %C.463: type = class_type @C, @C(%T.8b3, %empty_tuple.type) [symbolic]
-// CHECK:STDOUT:   %I.impl_witness.45b: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.8b3) [symbolic]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.8b3) [symbolic]
 // CHECK:STDOUT:   %T.826: %I.type = bind_symbolic_name T, 0 [symbolic]
 // CHECK:STDOUT:   %pattern_type.2b5: type = pattern_type %I.type [concrete]
 // CHECK:STDOUT:   %T.as_type: type = facet_access_type %T.826 [symbolic]
@@ -865,8 +845,6 @@ fn B() {
 // CHECK:STDOUT:   %B: %B.type = struct_value () [concrete]
 // CHECK:STDOUT:   %C.c74: type = class_type @C, @C(%empty_struct_type, %empty_struct_type) [concrete]
 // CHECK:STDOUT:   %C.val: %C.c74 = struct_value () [concrete]
-// CHECK:STDOUT:   %C.83b: type = class_type @C, @C(%empty_struct_type, %empty_tuple.type) [concrete]
-// CHECK:STDOUT:   %I.impl_witness.80e: <witness> = impl_witness file.%I.impl_witness_table, @impl(%empty_struct_type) [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {
@@ -904,7 +882,7 @@ fn B() {
 // CHECK:STDOUT:     %T.loc7_14.1: type = bind_symbolic_name T, 0 [symbolic = %T.loc7_14.2 (constants.%T.8b3)]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %I.impl_witness_table = impl_witness_table (), @impl [concrete]
-// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness %I.impl_witness_table, @impl(constants.%T.8b3) [symbolic = @impl.%I.impl_witness (constants.%I.impl_witness.45b)]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness %I.impl_witness_table, @impl(constants.%T.8b3) [symbolic = @impl.%I.impl_witness (constants.%I.impl_witness)]
 // CHECK:STDOUT:   %A.decl: %A.type = fn_decl @A [concrete = constants.%A] {
 // CHECK:STDOUT:     %T.patt: %pattern_type.2b5 = symbolic_binding_pattern T, 0 [concrete]
 // CHECK:STDOUT:     %t.patt: @A.%pattern_type (%pattern_type.6de) = binding_pattern t [concrete]
@@ -934,7 +912,7 @@ fn B() {
 // CHECK:STDOUT: generic impl @impl(%T.loc7_14.1: type) {
 // CHECK:STDOUT:   %T.loc7_14.2: type = bind_symbolic_name T, 0 [symbolic = %T.loc7_14.2 (constants.%T.8b3)]
 // CHECK:STDOUT:   %C.loc7_31.2: type = class_type @C, @C(%T.loc7_14.2, constants.%empty_tuple.type) [symbolic = %C.loc7_31.2 (constants.%C.463)]
-// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.loc7_14.2) [symbolic = %I.impl_witness (constants.%I.impl_witness.45b)]
+// CHECK:STDOUT:   %I.impl_witness: <witness> = impl_witness file.%I.impl_witness_table, @impl(%T.loc7_14.2) [symbolic = %I.impl_witness (constants.%I.impl_witness)]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !definition:
 // CHECK:STDOUT:
@@ -1004,7 +982,7 @@ fn B() {
 // CHECK:STDOUT: specific @impl(constants.%T.8b3) {
 // CHECK:STDOUT:   %T.loc7_14.2 => constants.%T.8b3
 // CHECK:STDOUT:   %C.loc7_31.2 => constants.%C.463
-// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness.45b
+// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: specific @A(constants.%T.826) {
@@ -1020,17 +998,6 @@ fn B() {
 // CHECK:STDOUT: !definition:
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: specific @impl(constants.%empty_struct_type) {
-// CHECK:STDOUT:   %T.loc7_14.2 => constants.%empty_struct_type
-// CHECK:STDOUT:   %C.loc7_31.2 => constants.%C.83b
-// CHECK:STDOUT:   %I.impl_witness => constants.%I.impl_witness.80e
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: specific @C(constants.%empty_struct_type, constants.%empty_tuple.type) {
-// CHECK:STDOUT:   %V.loc5_9.2 => constants.%empty_struct_type
-// CHECK:STDOUT:   %W.loc5_19.2 => constants.%empty_tuple.type
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
 // CHECK:STDOUT: --- include_files/convert.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {

+ 90 - 83
toolchain/check/type_structure.cpp

@@ -10,6 +10,7 @@
 
 #include "common/variant_helpers.h"
 #include "toolchain/check/context.h"
+#include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/type_iterator.h"
 
 namespace Carbon::Check {
@@ -21,6 +22,9 @@ auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
   const auto* lhs_cursor = lhs.begin();
   const auto* rhs_cursor = rhs.begin();
 
+  const auto* lhs_concrete_cursor = concrete_types_.begin();
+  const auto* rhs_concrete_cursor = other.concrete_types_.begin();
+
   while (true) {
     // If both structures end at the same time, they match.
     if (lhs_cursor == lhs.end() && rhs_cursor == rhs.end()) {
@@ -30,12 +34,21 @@ auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
     if (lhs_cursor == lhs.end() || rhs_cursor == rhs.end()) {
       return false;
     }
-    // Same structural element on both sides, they match and both are consumed.
-    //
-    // TODO: If we kept the constant value of the concrete element in the type
-    // structure, then we could compare them and use that to eliminate matching
-    // impls that are not actually compatible.
+    // Same structural element on both sides. Compare concrete values if
+    // possible to ensure they match. Both will be consumed.
     if (*lhs_cursor == *rhs_cursor) {
+      // Each Concrete and ConcreteOpenParen shape entry has a paired concrete
+      // value.
+      if (*lhs_cursor == Structural::Concrete ||
+          *lhs_cursor == Structural::ConcreteOpenParen) {
+        if (*lhs_concrete_cursor != *rhs_concrete_cursor) {
+          return false;
+        }
+
+        // Move past the shape and concrete value together.
+        ++lhs_concrete_cursor;
+        ++rhs_concrete_cursor;
+      }
       ++lhs_cursor;
       ++rhs_cursor;
       continue;
@@ -50,57 +63,15 @@ auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
     // From here we know one side is a Symbolic and the other is not. We can
     // match the Symbolic against either a single Concrete or a larger bracketed
     // set of Concrete structural elements.
-
-    // Returns false if the lhs and rhs can not match, true if we should
-    // continue checking for compatibility.
-    auto consume_symbolic = [](const auto*& lhs_cursor,
-                               const auto*& rhs_cursor) -> bool {
-      // Consume the symbolic on the RHS.
-      ++rhs_cursor;
-
-      // The symbolic on the RHS is in the same position as a close paren on the
-      // LHS, which means the structures can not match.
-      //
-      // Example:
-      // - ((c))
-      // - ((c?))
-      if (*lhs_cursor == Structural::ConcreteCloseParen) {
-        return false;
-      }
-
-      // There's either a Concrete element or an open paren on the LHS. If it's
-      // the former, the Symbolic just matches with it. If it's the latter, the
-      // Symbolic matches with everything on the LHS up to the matching closing
-      // paren.
-      CARBON_CHECK(*lhs_cursor == Structural::Concrete ||
-                   *lhs_cursor == Structural::ConcreteOpenParen);
-      int depth = 0;
-      do {
-        switch (*lhs_cursor) {
-          case Structural::ConcreteOpenParen:
-            depth += 1;
-            break;
-          case Structural::ConcreteCloseParen:
-            depth -= 1;
-            break;
-          case Structural::Concrete:
-            break;
-          case Structural::Symbolic:
-            break;
-        }
-        ++lhs_cursor;
-      } while (depth > 0);
-      return true;
-    };
-
+    //
     // We move the symbolic to the RHS to make only one case to handle in the
     // lambda.
     if (*lhs_cursor == Structural::Symbolic) {
-      if (!consume_symbolic(rhs_cursor, lhs_cursor)) {
+      if (!ConsumeRhsSymbolic(rhs_cursor, rhs_concrete_cursor, lhs_cursor)) {
         return false;
       }
     } else {
-      if (!consume_symbolic(lhs_cursor, rhs_cursor)) {
+      if (!ConsumeRhsSymbolic(lhs_cursor, lhs_concrete_cursor, rhs_cursor)) {
         return false;
       }
     }
@@ -109,6 +80,56 @@ auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
   return true;
 }
 
+// Returns false if the lhs and rhs can not match, true if we should
+// continue checking for compatibility.
+auto TypeStructure::ConsumeRhsSymbolic(
+    llvm::SmallVector<Structural>::const_iterator& lhs_cursor,
+    llvm::SmallVector<ConcreteType>::const_iterator& lhs_concrete_cursor,
+    llvm::SmallVector<Structural>::const_iterator& rhs_cursor) -> bool {
+  // Consume the symbolic on the RHS.
+  ++rhs_cursor;
+
+  // The symbolic on the RHS is in the same position as a close paren on the
+  // LHS, which means the structures can not match.
+  //
+  // Example:
+  // - ((c))
+  // - ((c?))
+  if (*lhs_cursor == TypeStructure::Structural::ConcreteCloseParen) {
+    return false;
+  }
+
+  // There's either a Concrete element or an open paren on the LHS. If it's
+  // the former, the Symbolic just matches with it. If it's the latter, the
+  // Symbolic matches with everything on the LHS up to the matching closing
+  // paren.
+  CARBON_CHECK(*lhs_cursor == Structural::Concrete ||
+               *lhs_cursor == Structural::ConcreteOpenParen);
+  int depth = 0;
+  do {
+    switch (*lhs_cursor) {
+      case Structural::ConcreteOpenParen:
+        depth += 1;
+        // Each Concrete and ConcreteOpenParen shape entry has a paired
+        // concrete value. Skip the shape and concrete value together.
+        ++lhs_concrete_cursor;
+        break;
+      case Structural::ConcreteCloseParen:
+        depth -= 1;
+        break;
+      case Structural::Concrete:
+        // Each Concrete and ConcreteOpenParen shape entry has a paired
+        // concrete value. Skip the shape and concrete value together.
+        ++lhs_concrete_cursor;
+        break;
+      case Structural::Symbolic:
+        break;
+    }
+    ++lhs_cursor;
+  } while (depth > 0);
+  return true;
+}
+
 // A class that builds a `TypeStructure` for an `Impl`, or an impl lookup query,
 // that represents its self type and interface.
 class TypeStructureBuilder {
@@ -188,14 +209,15 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
         [&](SemIR::TypeIterator::Step::TemplateType) {
           AppendStructuralSymbolic();
         },
-        [&](SemIR::TypeIterator::Step::Value) {
-          // TODO: Include the value's type into the structure, with the type
-          // coming first and paired together with the value, like:
-          // `{TypeWithPossibleNestedTypes, Concrete}`.
-          // We might want a different bracket marker than ConcreteOpenParen
-          // for this so that it can look different in the type structure when
-          // dumped.
-          AppendStructuralConcrete(SemIR::ErrorInst::TypeId);
+        [&](SemIR::TypeIterator::Step::ConcreteValue value) {
+          AppendStructuralConcrete(
+              context_->constant_values().Get(value.inst_id));
+        },
+        [&](SemIR::TypeIterator::Step::SymbolicValue) {
+          AppendStructuralSymbolic();
+        },
+        [&](SemIR::TypeIterator::Step::StructFieldName field_name) {
+          AppendStructuralConcrete(field_name.name_id);
         },
         [&](SemIR::TypeIterator::Step::StartOnly start) {
           VariantMatch(
@@ -203,8 +225,8 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
               [&](SemIR::TypeIterator::Step::ClassStart class_start) {
                 AppendStructuralConcrete(class_start.class_id);
               },
-              [&](SemIR::TypeIterator::Step::StructStart struct_start) {
-                AppendStructuralConcrete(struct_start.type_id);
+              [&](SemIR::TypeIterator::Step::StructStart) {
+                AppendStructuralConcrete(TypeStructure::ConcreteStructType());
               },
               [&](SemIR::TypeIterator::Step::TupleStart) {
                 AppendStructuralConcrete(TypeStructure::ConcreteTupleType());
@@ -220,19 +242,11 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
           VariantMatch(
               start_with_end.any,
               [&](SemIR::TypeIterator::Step::ClassStart class_start) {
-                // TODO: We should be able to use `class_id` here but non-type
-                // values are not differentiated right now. So `Int(32)` and
-                // `Int(16)` and `Int(N)` all look the same. We need to make
-                // this change in order to use the concrete types in type
-                // structure comparison for impl matching.
-                AppendStructuralConcreteOpenParen(class_start.type_id);
+                AppendStructuralConcreteOpenParen(class_start.class_id);
               },
-              [&](SemIR::TypeIterator::Step::StructStart struct_start) {
-                // TODO: Should we use a `TypeStructure::ConcreteStructType`
-                // here? To do so we need to include the struct field names in
-                // the concrete elements of the type structure to
-                // differentiate different struct types.
-                AppendStructuralConcreteOpenParen(struct_start.type_id);
+              [&](SemIR::TypeIterator::Step::StructStart) {
+                AppendStructuralConcreteOpenParen(
+                    TypeStructure::ConcreteStructType());
               },
               [&](SemIR::TypeIterator::Step::TupleStart) {
                 AppendStructuralConcreteOpenParen(
@@ -241,20 +255,13 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
               [&](SemIR::TypeIterator::Step::InterfaceStart interface_start) {
                 AppendStructuralConcreteOpenParen(interface_start.interface_id);
               },
-              [&](SemIR::TypeIterator::Step::ArrayStart start) {
-                AppendStructuralConcreteOpenParen(
-                    // TODO: We should be able to use
-                    // `TypeStructure::ConcreteArrayType()` here but non-type
-                    // values are not differentiated right now. So
-                    // `array(i32, 1)` and `array(i32, 2)` both look the same.
-                    // We need to make this change in order to use the
-                    // concrete types in type structure comparison for impl
-                    // matching.
-                    start.type_id);
-              },
               [&](SemIR::TypeIterator::Step::IntStart int_start) {
                 AppendStructuralConcreteOpenParen(int_start.type_id);
               },
+              [&](SemIR::TypeIterator::Step::ArrayStart) {
+                AppendStructuralConcreteOpenParen(
+                    TypeStructure::ConcreteArrayType());
+              },
               [&](SemIR::TypeIterator::Step::PointerStart) {
                 AppendStructuralConcreteOpenParen(
                     TypeStructure::ConcretePointerType());

+ 26 - 2
toolchain/check/type_structure.h

@@ -101,11 +101,22 @@ class TypeStructure : public Printable<TypeStructure> {
     Symbolic,
   };
 
+  // Marks an array type. The type and bound will appear as other entries.
+  struct ConcreteArrayType {
+    friend auto operator==(ConcreteArrayType /*lhs*/, ConcreteArrayType /*rhs*/)
+        -> bool = default;
+  };
   // Marks a pointer type. The pointee type will appear as another entry.
   struct ConcretePointerType {
     friend auto operator==(ConcretePointerType /*lhs*/,
                            ConcretePointerType /*rhs*/) -> bool = default;
   };
+  // Marks a struct type. The field names and types will appear as other
+  // entries.
+  struct ConcreteStructType {
+    friend auto operator==(ConcreteStructType /*lhs*/,
+                           ConcreteStructType /*rhs*/) -> bool = default;
+  };
   // Marks a tuple type. The type members (if any) will appear as other entries.
   struct ConcreteTupleType {
     friend auto operator==(ConcreteTupleType /*lhs*/, ConcreteTupleType /*rhs*/)
@@ -114,9 +125,16 @@ class TypeStructure : public Printable<TypeStructure> {
   // The `concrete_types_` tracks the specific concrete type for each
   // `Structural::Concrete` or `Structural::ConcreteOpenParen` in the type
   // structure.
+  //
+  // `ConstantId` is used strictly for non-type values. For types, `TypeId` is
+  // used.
+  //
+  // `NameId` is used strictly for struct fields, as the field names are part of
+  // the struct type.
   using ConcreteType =
-      std::variant<ConcretePointerType, ConcreteTupleType, SemIR::TypeId,
-                   SemIR::ClassId, SemIR::InterfaceId>;
+      std::variant<ConcreteArrayType, ConcretePointerType, ConcreteStructType,
+                   ConcreteTupleType, SemIR::ClassId, SemIR::ConstantId,
+                   SemIR::InterfaceId, SemIR::NameId, SemIR::TypeId>;
 
   TypeStructure(llvm::SmallVector<Structural> structure,
                 llvm::SmallVector<int> symbolic_type_indices,
@@ -125,6 +143,12 @@ class TypeStructure : public Printable<TypeStructure> {
         symbolic_type_indices_(std::move(symbolic_type_indices)),
         concrete_types_(std::move(concrete_types)) {}
 
+  // A helper for IsCompatibleWith.
+  static auto ConsumeRhsSymbolic(
+      llvm::SmallVector<Structural>::const_iterator& lhs_cursor,
+      llvm::SmallVector<ConcreteType>::const_iterator& lhs_concrete_cursor,
+      llvm::SmallVector<Structural>::const_iterator& rhs_cursor) -> bool;
+
   // The structural position of concrete and symbolic values in the type.
   llvm::SmallVector<Structural> structure_;
 

+ 18 - 5
toolchain/sem_ir/type_iterator.cpp

@@ -19,6 +19,10 @@ auto TypeIterator::Next() -> Step {
     auto next = work_list_.back();
     work_list_.pop_back();
 
+    // TODO: Consider using a CARBON_KIND_SWITCH on `next` here after
+    // https://github.com/carbon-language/carbon-lang/pull/5433 arrives, instead
+    // of a bunch of `if` conditions.
+
     if (std::holds_alternative<EndType>(next)) {
       return Step::End();
     }
@@ -40,8 +44,16 @@ auto TypeIterator::Next() -> Step {
       return Step::SymbolicType{.facet_type_id = symbolic->facet_type_id};
     }
 
-    if (std::holds_alternative<NonTypeValue>(next)) {
-      return Step::Value();
+    if (const auto* value = std::get_if<ConcreteNonTypeValue>(&next)) {
+      return Step::ConcreteValue{.inst_id = value->inst_id};
+    }
+
+    if (const auto* value = std::get_if<SymbolicNonTypeValue>(&next)) {
+      return Step::SymbolicValue{.inst_id = value->inst_id};
+    }
+
+    if (const auto* value = std::get_if<StructFieldName>(&next)) {
+      return Step::StructFieldName{.name_id = value->name_id};
     }
 
     SemIR::TypeId type_id = std::get<SemIR::TypeId>(next);
@@ -141,8 +153,7 @@ auto TypeIterator::Next() -> Step {
         } else {
           Push(EndType());
           for (const auto& field : llvm::reverse(fields)) {
-            // TODO: Are struct field names part of the type structure? They
-            // are part of a struct's type.
+            Push(StructFieldName{.name_id = field.name_id});
             PushInstId(field.type_inst_id);
           }
           return Step::StartWithEnd(Step::StructStart{.type_id = type_id});
@@ -215,8 +226,10 @@ auto TypeIterator::PushInstId(SemIR::InstId inst_id) -> void {
   } else if (auto type_id = std::get<SemIR::TypeId>(maybe_type_id);
              type_id.has_value()) {
     Push(type_id);
+  } else if (sem_ir_->constant_values().Get(inst_id).is_symbolic()) {
+    Push(SymbolicNonTypeValue{.inst_id = inst_id});
   } else {
-    Push(NonTypeValue{.type_id = sem_ir_->insts().Get(inst_id).type_id()});
+    Push(ConcreteNonTypeValue{.inst_id = inst_id});
   }
 }
 

+ 58 - 23
toolchain/sem_ir/type_iterator.h

@@ -59,14 +59,23 @@ class TypeIterator {
   struct SymbolicType {
     SemIR::TypeId facet_type_id;
   };
-  // A work item to mark a non-type value.
-  struct NonTypeValue {
-    // The type of the value.
-    SemIR::TypeId type_id;
+  // A work item to mark a concrete non-type value.
+  struct ConcreteNonTypeValue {
+    SemIR::InstId inst_id;
+  };
+  // A work item to mark a symbolic non-type value.
+  struct SymbolicNonTypeValue {
+    SemIR::InstId inst_id;
+  };
+  // A work item to mark the name of a struct field.
+  struct StructFieldName {
+    SemIR::NameId name_id;
   };
 
-  using WorkItem = std::variant<SemIR::TypeId, SymbolicType, NonTypeValue,
-                                SemIR::SpecificInterface, EndType>;
+  using WorkItem =
+      std::variant<SemIR::TypeId, SymbolicType, ConcreteNonTypeValue,
+                   SymbolicNonTypeValue, StructFieldName,
+                   SemIR::SpecificInterface, EndType>;
 
   // Get the TypeId for an instruction that is not a facet value, otherwise
   // return SymbolicType to indicate the instruction is a symbolic facet value.
@@ -97,6 +106,10 @@ class TypeIterator {
 
 class TypeIterator::Step {
  public:
+  // ===========================================================================
+  // Results that enter scope where the following results are related. These
+  // only appear in `StartOnly` or `StartWithEnd`.
+
   // Followed by generic parameters.
   struct ClassStart {
     SemIR::ClassId class_id;
@@ -125,21 +138,8 @@ class TypeIterator::Step {
   // Followed by the pointee type.
   struct PointerStart {};
 
-  // A type value.
-  struct ConcreteType {
-    SemIR::TypeId type_id;
-  };
-  // A symbolic type value, constrained by `facet_type_id`.
-  struct SymbolicType {
-    // Either a FacetType or the TypeType singleton.
-    SemIR::TypeId facet_type_id;
-  };
-  // A symbolic template type value.
-  struct TemplateType {};
-  // A non-type value, which can be found as a generic parameter for a type.
-  struct Value {
-    // TODO: Include the type of the value somewhere.
-  };
+  // ===========================================================================
+  // Aggregator results that begin a scope and potentially end it immediately.
 
   // A *Start type, but without anything following it, so we also skip the
   // matching `End`.
@@ -156,14 +156,49 @@ class TypeIterator::Step {
     Any any;
   };
 
+  // ===========================================================================
+  // Individual result values, which appear on their own or inside some scope
+  // that begin with `StartWithEnd`.
+
+  // A type value.
+  struct ConcreteType {
+    SemIR::TypeId type_id;
+  };
+  // A symbolic type value, constrained by `facet_type_id`.
+  struct SymbolicType {
+    // Either a FacetType or the TypeType singleton.
+    SemIR::TypeId facet_type_id;
+  };
+  // A symbolic template type value.
+  struct TemplateType {};
+  // A concrete non-type value, which can be found as a generic parameter for a
+  // type.
+  struct ConcreteValue {
+    // An instruction that evaluates to the constant value.
+    SemIR::InstId inst_id;
+  };
+  // A symbolic non-type value, which can be found as a generic parameter for a
+  // type.
+  struct SymbolicValue {
+    // An instruction that evaluates to the constant value.
+    SemIR::InstId inst_id;
+  };
+  // A struct field name. The field names contribute to the type of the struct.
+  struct StructFieldName {
+    SemIR::NameId name_id;
+  };
+
+  // ===========================================================================
+  // Results that report a state change in iteration.
+
   // Closes the scope of a `StartWithEnd`.
   struct End {};
-
   // Iteration is complete.
   struct Done {};
 
   // Each step is one of these.
-  using Any = std::variant<ConcreteType, SymbolicType, TemplateType, Value,
+  using Any = std::variant<ConcreteType, SymbolicType, TemplateType,
+                           ConcreteValue, SymbolicValue, StructFieldName,
                            StartOnly, StartWithEnd, End, Done>;
 
   template <class T>