Browse Source

Some trivial check-phase inlining. (#4362)

When profiling, these jumped out as good inline candidates that happened
to be out-of-line, this just moves them inline so that they're
available. I think as much as 10% improvement in check-phase from this,
but I haven't run detailed before/after measurements as these changes
seemed minimally disruptive.

Also switched from `CHECK` to `DCHECK` in one place that seems
especially hot and where the check itself seems reasonable to only do in
debug builds. Left a comment since we rarely need to remove these any
more.
Chandler Carruth 1 year ago
parent
commit
55d8edcdc7
3 changed files with 25 additions and 33 deletions
  1. 4 1
      toolchain/base/value_store.h
  2. 0 28
      toolchain/check/context.cpp
  3. 21 4
      toolchain/check/context.h

+ 4 - 1
toolchain/base/value_store.h

@@ -151,7 +151,10 @@ class ValueStore
   // Stores the value and returns an ID to reference it.
   auto Add(ValueType value) -> IdT {
     IdT id(values_.size());
-    CARBON_CHECK(id.index >= 0, "Id overflow");
+    // This routine is especially hot and the check here relatively expensive
+    // for the value provided, so only do this in debug builds to make tracking
+    // down issues easier.
+    CARBON_DCHECK(id.index >= 0, "Id overflow");
     values_.push_back(std::move(value));
     return id;
   }

+ 0 - 28
toolchain/check/context.cpp

@@ -150,20 +150,6 @@ auto Context::CheckCompatibleImportedNodeKind(
       kind, imported_kind);
 }
 
-auto Context::AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst)
-    -> SemIR::InstId {
-  auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst);
-  CARBON_VLOG("AddInst: {0}\n", loc_id_and_inst.inst);
-  FinishInst(inst_id, loc_id_and_inst.inst);
-  return inst_id;
-}
-
-auto Context::AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId {
-  auto inst_id = AddInstInNoBlock(loc_id_and_inst);
-  inst_block_stack_.AddInstId(inst_id);
-  return inst_id;
-}
-
 auto Context::AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst)
     -> SemIR::InstId {
   auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst);
@@ -179,20 +165,6 @@ auto Context::AddPlaceholderInst(SemIR::LocIdAndInst loc_id_and_inst)
   return inst_id;
 }
 
-auto Context::AddPatternInst(SemIR::LocIdAndInst loc_id_and_inst)
-    -> SemIR::InstId {
-  auto inst_id = AddInstInNoBlock(loc_id_and_inst);
-  pattern_block_stack_.AddInstId(inst_id);
-  return inst_id;
-}
-
-auto Context::AddConstant(SemIR::Inst inst, bool is_symbolic)
-    -> SemIR::ConstantId {
-  auto const_id = constants().GetOrAdd(inst, is_symbolic);
-  CARBON_VLOG("AddConstant: {0}\n", inst);
-  return const_id;
-}
-
 auto Context::ReplaceLocIdAndInstBeforeConstantUse(
     SemIR::InstId inst_id, SemIR::LocIdAndInst loc_id_and_inst) -> void {
   sem_ir().insts().SetLocIdAndInst(inst_id, loc_id_and_inst);

+ 21 - 4
toolchain/check/context.h

@@ -82,7 +82,11 @@ class Context {
   auto VerifyOnFinish() -> void;
 
   // Adds an instruction to the current block, returning the produced ID.
-  auto AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
+  auto AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId {
+    auto inst_id = AddInstInNoBlock(loc_id_and_inst);
+    inst_block_stack_.AddInstId(inst_id);
+    return inst_id;
+  }
 
   // Convenience for AddInst with typed nodes.
   template <typename InstT, typename LocT>
@@ -106,7 +110,12 @@ class Context {
 
   // Adds an instruction in no block, returning the produced ID. Should be used
   // rarely.
-  auto AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
+  auto AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId {
+    auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst);
+    CARBON_VLOG("AddInst: {0}\n", loc_id_and_inst.inst);
+    FinishInst(inst_id, loc_id_and_inst.inst);
+    return inst_id;
+  }
 
   // Convenience for AddInstInNoBlock with typed nodes.
   template <typename InstT, typename LocT>
@@ -128,7 +137,11 @@ class Context {
 
   // Adds an instruction to the current pattern block, returning the produced
   // ID.
-  auto AddPatternInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
+  auto AddPatternInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId {
+    auto inst_id = AddInstInNoBlock(loc_id_and_inst);
+    pattern_block_stack_.AddInstId(inst_id);
+    return inst_id;
+  }
 
   // Convenience for AddPatternInst with typed nodes.
   template <typename InstT>
@@ -139,7 +152,11 @@ class Context {
   }
 
   // Adds an instruction to the constants block, returning the produced ID.
-  auto AddConstant(SemIR::Inst inst, bool is_symbolic) -> SemIR::ConstantId;
+  auto AddConstant(SemIR::Inst inst, bool is_symbolic) -> SemIR::ConstantId {
+    auto const_id = constants().GetOrAdd(inst, is_symbolic);
+    CARBON_VLOG("AddConstant: {0}\n", inst);
+    return const_id;
+  }
 
   // Pushes a parse tree node onto the stack, storing the SemIR::Inst as the
   // result.