diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 44fcccb4056e..1e83792f56ab 100644 --- a/toolchain/check/convert.cpp +++ b/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, diff --git a/toolchain/check/convert.h b/toolchain/check/convert.h index 0e9a860450a2..b73f29e55563 100644 --- a/toolchain/check/convert.h +++ b/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 diff --git a/toolchain/check/handle_loop_statement.cpp b/toolchain/check/handle_loop_statement.cpp index 89d6353b99d4..4ac6e5c5aa2f 100644 --- a/toolchain/check/handle_loop_statement.cpp +++ b/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, diff --git a/toolchain/check/pending_block.h b/toolchain/check/pending_block.h index 9ef76d858911..199e7b603dce 100644 --- a/toolchain/check/pending_block.h +++ b/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;