Clarify and partially enforce inst-order precondition on splicing (#6722)

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

toolchain/check/convert.cpp

@@ -1897,6 +1897,8 @@ auto Initialize(Context& context, SemIR::LocId loc_id, SemIR::InstId storage_id,
     // TODO: is it safe to use storage_id when the init repr is dependent?
     storage_id = SemIR::InstId::None;
   }
+  // TODO: add CHECK that storage_id.index < value_id.index to enforce the
+  // precondition, once existing violations have been cleaned up.
   PendingBlock target_block(&context);
   return Convert(context, loc_id, value_id,
                  {.kind = ConversionTarget::Initializing,

toolchain/check/convert.h

@@ -64,6 +64,7 @@ struct ConversionTarget {
   SemIR::InstId storage_id = SemIR::InstId::None;
   // For an initializer, a block of pending instructions that `storage_id`
   // depends on, and that can be discarded if `storage_id` is not accessed.
+  // If this is not null or empty, its last element must be storage_id.
   PendingBlock* storage_access_block = nullptr;
   // Whether failure of conversion is an error and is diagnosed to the user.
   // When looking for a possible conversion but with graceful fallback, diagnose
@@ -93,7 +94,8 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
 // Converts `value_id` to an initializing expression of the type of
 // `storage_id`, and returns the possibly-converted initializing expression.
 // `storage_id` is used as the storage argument of the resulting expression
-// except as noted below. The caller is responsible for passing the result to an
+// except as noted below, and when it is used as the storage argument it must
+// precede `value_id`. The caller is responsible for passing the result to an
 // inst that is documented as consuming it, such as `Assign`.
 //
 // `for_return` indicates that this conversion is initializing the operand of a

toolchain/check/handle_loop_statement.cpp

@@ -160,6 +160,9 @@ auto HandleParseNode(Context& context, Parse::ForHeaderId node_id) -> bool {
   // Create the cursor variable.
   // TODO: Produce a custom diagnostic if the range operand can't be used as a
   // range.
+  // TODO: We need to allocate the `VarStorage` before building the operator.
+  // The current order risks violating the preconditions on `Initialize` and
+  // risks violating the topological ordering of insts.
   auto cursor_id =
       BuildUnaryOperator(context, node_id,
                          {.interface_name = CoreIdentifier::Iterate,

toolchain/check/pending_block.h

@@ -69,9 +69,20 @@ class PendingBlock {
 
   // Replace the instruction at target_id with the instructions in this block.
   // The new value for target_id should be value_id. Returns the InstId that
-  // should be used to refer to the result from now on.
+  // should be used to refer to the result from now on. value_id must precede
+  // target_id, or be the last ID in this block, in order to preserve the
+  // property that SemIR is topologically sorted.
+  //
+  // TODO: we could also allow value_id to be one of the other insts in this
+  // block, but that would be costlier to enforce.
   auto MergeReplacing(SemIR::InstId target_id, SemIR::InstId value_id)
       -> SemIR::InstId {
+    // TODO: consider adding an end-of-phase check that the SemIR::File is in
+    // SSA form, and dropping this check and the ordering preconditions here and
+    // on Initialize.
+    CARBON_CHECK(value_id.index <= target_id.index ||
+                     (!insts_.empty() && insts_.back() == value_id),
+                 "Splice would break topological sorting of insts");
     SemIR::LocIdAndInst value = context_->insts().GetWithLocId(value_id);
 
     auto result_id = value_id;