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

Check that constants are lowered in the proper order. (#6566)

Instead of comparing `InstId` indexes, which aren't *necessarily* in the
same order as raw indexes, compare the raw indexes themselves. Convert
the test for out-of-order lowering into a `CHECK` failure if a constant
is found to refer to another constant with a later-created instruction.

In principle this is fixing a bug: if there were so many files and
instructions that the bits of the tag overlapped the bits of the
`InstId`, we could return `nullptr` for a constant that actually had a
value. But in practice this would be very hard to test, and even harder
to test reliably, so I'm not including a test here. The purpose of this
change is to add the `CHECK`, not to fix an obscure bug.
Richard Smith 3 месяцев назад
Родитель
Сommit
9727c628c4
1 измененных файлов с 19 добавлено и 11 удалено
  1. 19 11
      toolchain/lower/constant.cpp

+ 19 - 11
toolchain/lower/constant.cpp

@@ -35,12 +35,13 @@ class ConstantContext {
   auto GetConstant(SemIR::ConstantId const_id) const -> llvm::Constant* {
     CARBON_CHECK(const_id.is_concrete(), "Unexpected constant ID {0}",
                  const_id);
-    auto inst_id =
-        file_context_->sem_ir().constant_values().GetInstId(const_id);
-    if (inst_id.index > last_lowered_constant_index_) {
-      // This constant hasn't been lowered.
-      return nullptr;
-    }
+    auto inst_id = sem_ir().constant_values().GetInstId(const_id);
+    CARBON_CHECK(sem_ir().insts().GetRawIndex(inst_id) <
+                     sem_ir().insts().GetRawIndex(current_constant_inst_id_),
+                 "Accessed constants out of order: {0} {1} uses {2} {3}",
+                 current_constant_inst_id_,
+                 sem_ir().insts().Get(current_constant_inst_id_), inst_id,
+                 sem_ir().insts().Get(inst_id));
     return constants_->Get(inst_id);
   }
 
@@ -80,10 +81,15 @@ class ConstantContext {
     return file_context_->BuildGlobalVariableDecl(inst);
   }
 
-  // Sets the index of the constant we most recently lowered. This is used to
+  // Sets the InstId of the constant we're currently lowering. This is used to
   // check we don't look at constants that we've not lowered yet.
-  auto SetLastLoweredConstantIndex(int32_t index) -> void {
-    last_lowered_constant_index_ = index;
+  auto SetCurrentConstantInstId(SemIR::InstId inst_id) -> void {
+    CARBON_CHECK(
+        !current_constant_inst_id_.has_value() ||
+            sem_ir().insts().GetRawIndex(inst_id) >
+                sem_ir().insts().GetRawIndex(current_constant_inst_id_),
+        "Visited constants out of order");
+    current_constant_inst_id_ = inst_id;
   }
 
   auto llvm_context() const -> llvm::LLVMContext& {
@@ -97,7 +103,7 @@ class ConstantContext {
  private:
   FileContext* file_context_;
   const FileContext::LoweredConstantStore* constants_;
-  int32_t last_lowered_constant_index_ = -1;
+  SemIR::InstId current_constant_inst_id_ = SemIR::InstId::None;
 };
 
 // Emits an aggregate constant of LLVM type `Type` whose elements are the
@@ -344,6 +350,9 @@ auto LowerConstants(FileContext& file_context,
       // but didn't actually use it.
       continue;
     }
+
+    context.SetCurrentConstantInstId(inst_id);
+
     llvm::Constant* value = nullptr;
     CARBON_KIND_SWITCH(inst) {
 #define CARBON_SEM_IR_INST_KIND(Name)                 \
@@ -355,7 +364,6 @@ auto LowerConstants(FileContext& file_context,
     }
 
     constants.Set(inst_id, value);
-    context.SetLastLoweredConstantIndex(inst_id.index);
   }
 }