Rename rewrite_value to rewrite_inst_id to clarify what it's holding (#5286)

Also add a note about the RewriteConstraint insts being canonical

toolchain/check/facet_type.cpp

@@ -134,8 +134,8 @@ auto InitialFacetTypeImplWitness(
       // for it to use to attempt recovery.
       continue;
     }
-    auto rewrite_value = rewrite.rhs_id;
-    if (rewrite_value == SemIR::ErrorInst::SingletonInstId) {
+    auto rewrite_inst_id = rewrite.rhs_id;
+    if (rewrite_inst_id == SemIR::ErrorInst::SingletonInstId) {
       table_entry = SemIR::ErrorInst::SingletonInstId;
       continue;
     }
@@ -165,9 +165,9 @@ auto InitialFacetTypeImplWitness(
     }
 
     if (table_entry != SemIR::ImplWitnessTablePlaceholder::SingletonInstId) {
-      if (table_entry != rewrite_value) {
+      if (table_entry != rewrite_inst_id) {
         // TODO: Figure out how to print the two different values
-        // `const_id` & `rewrite_value` in the diagnostic
+        // `const_id` & `rewrite_inst_id` in the diagnostic
         // message.
         CARBON_DIAGNOSTIC(AssociatedConstantWithDifferentValues, Error,
                           "associated constant {0} given two different values",
@@ -196,16 +196,16 @@ auto InitialFacetTypeImplWitness(
       // forming the facet type because the type of the associated constant
       // was symbolic.
       auto converted_inst_id = ConvertToValueOfType(
-          context, context.insts().GetLocId(facet_type_inst_id), rewrite_value,
-          assoc_const_type_id);
+          context, context.insts().GetLocId(facet_type_inst_id),
+          rewrite_inst_id, assoc_const_type_id);
       // Canonicalize the converted constant value.
       converted_inst_id =
           context.constant_values().GetConstantInstId(converted_inst_id);
       // The result of conversion can be non-constant even if the original
       // value was constant.
       if (converted_inst_id.has_value()) {
-        rewrite_value = converted_inst_id;
-      } else if (rewrite_value != SemIR::ErrorInst::SingletonInstId) {
+        rewrite_inst_id = converted_inst_id;
+      } else if (rewrite_inst_id != SemIR::ErrorInst::SingletonInstId) {
         const auto& assoc_const = context.associated_constants().Get(
             assoc_constant_decl->assoc_const_id);
         CARBON_DIAGNOSTIC(
@@ -216,18 +216,18 @@ auto InitialFacetTypeImplWitness(
         context.emitter().Emit(facet_type_inst_id,
                                AssociatedConstantNotConstantAfterConversion,
                                assoc_const.name_id, assoc_const_type_id);
-        rewrite_value = SemIR::ErrorInst::SingletonInstId;
+        rewrite_inst_id = SemIR::ErrorInst::SingletonInstId;
       }
     }
 
-    CARBON_CHECK(rewrite_value ==
-                     context.constant_values().GetConstantInstId(rewrite_value),
+    CARBON_CHECK(rewrite_inst_id == context.constant_values().GetConstantInstId(
+                                        rewrite_inst_id),
                  "Rewritten value for associated constant is not canonical.");
 
     table_entry = AddInst<SemIR::ImplWitnessAssociatedConstant>(
         context, witness_loc_id,
-        {.type_id = context.insts().Get(rewrite_value).type_id(),
-         .inst_id = rewrite_value});
+        {.type_id = context.insts().Get(rewrite_inst_id).type_id(),
+         .inst_id = rewrite_inst_id});
   }
   return witness_inst_id;
 }

toolchain/sem_ir/facet_type_info.h

@@ -33,7 +33,11 @@ struct FacetTypeInfo : Printable<FacetTypeInfo> {
   // These are the required interfaces that are not lookup contexts.
   llvm::SmallVector<ImplsConstraint> self_impls_constraints;
 
-  // Rewrite constraints of the form `.T = U`
+  // Rewrite constraints of the form `.T = U`.
+  //
+  // The InstIds here must be canonical instructions (which come from the
+  // instruction in a constant value) in order to ensure comparison works
+  // correctly.
   struct RewriteConstraint {
     InstId lhs_id;
     InstId rhs_id;