From 3a489fd7b4642a600e456f192bc23de817f5ec85 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 1 Feb 2024 20:23:56 -0800 Subject: [PATCH] Implement ConstantId::Invalid (#3674) Right now, ConstantValueStore defaults to having unknown values use NotConstant. This generally works for the current IR, but with imports we're expecting sparse entries which are generally unknown -- and distinguishing between NotConstant and simply unset would be helpful. As a consequence, add Invalid. We discussed whether to simply have ConstantValueStore default to Invalid going forward, or to make the default flexible. The upside to the former is consistency, the upside to the latter is that it should result in fewer Sets when operating on the current IR (which will more frequently have known non-constant values). This PR offers both approaches in separate commits, but I somewhat lean towards the latter for fewer array resizes. Note, a totally different approach would be to use a different class (not ConstantValueStore) for imported IRs -- then the default of Invalid versus NotConstant would be type-dependent. However, I expect we're going to want to do at least somewhat consistent lookups, and using the same ConstantValueStore for both cases allows avoiding a virtual interface or templating. Also, I'm hoping to only maintain the ConstantValueStore for an imported IR as part of Context (not File), which would mean the SmallVector storage overhead is ephemeral, mitigating one of the potential advantages of using a different type for imported IRs. --- toolchain/base/index_base.h | 4 +-- toolchain/check/context.cpp | 1 + toolchain/check/eval.cpp | 1 + toolchain/sem_ir/file.cpp | 1 + toolchain/sem_ir/formatter.cpp | 24 +++++++++++---- toolchain/sem_ir/ids.h | 53 ++++++++++++++++++++++----------- toolchain/sem_ir/value_stores.h | 13 +++++--- 7 files changed, 69 insertions(+), 28 deletions(-) diff --git a/toolchain/base/index_base.h b/toolchain/base/index_base.h index b94ea514ab3c..887cccba2bbd 100644 --- a/toolchain/base/index_base.h +++ b/toolchain/base/index_base.h @@ -39,7 +39,7 @@ struct IdBase : public Printable { } } - auto is_valid() const -> bool { return index != InvalidIndex; } + constexpr auto is_valid() const -> bool { return index != InvalidIndex; } int32_t index; }; @@ -58,7 +58,7 @@ struct IndexBase : public IdBase { // convertible to that type. template requires std::derived_from -auto operator==(IndexType lhs, IndexType rhs) -> bool { +constexpr auto operator==(IndexType lhs, IndexType rhs) -> bool { return lhs.index == rhs.index; } template diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index a67be493fd15..2430a9c5f524 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -92,6 +92,7 @@ auto Context::AddPlaceholderInstInNoBlock( SemIR::ParseNodeAndInst parse_node_and_inst) -> SemIR::InstId { auto inst_id = sem_ir().insts().AddInNoBlock(parse_node_and_inst); CARBON_VLOG() << "AddPlaceholderInst: " << parse_node_and_inst.inst << "\n"; + constant_values().Set(inst_id, SemIR::ConstantId::Invalid); return inst_id; } diff --git a/toolchain/check/eval.cpp b/toolchain/check/eval.cpp index 1cf484765fad..9d4fe203c1a4 100644 --- a/toolchain/check/eval.cpp +++ b/toolchain/check/eval.cpp @@ -40,6 +40,7 @@ static auto GetPhase(SemIR::ConstantId constant_id) -> Phase { } else if (constant_id.is_template()) { return Phase::Template; } else { + CARBON_CHECK(constant_id.is_symbolic()); return Phase::Symbolic; } } diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index 9a183a0c7478..24669973e7a4 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -65,6 +65,7 @@ File::File(SharedValueStores& value_stores, std::string filename, : value_stores_(&value_stores), filename_(std::move(filename)), type_blocks_(allocator_), + constant_values_(ConstantId::NotConstant), inst_blocks_(allocator_), constants_(*this, allocator_) { CARBON_CHECK(builtins != nullptr); diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index d872da8702d8..d61b22542a95 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -772,17 +772,31 @@ class Formatter { out_ << InstT::Kind.ir_name(); FormatInstructionRHS(inst); if (auto const_id = sem_ir_.constant_values().Get(inst_id); - const_id.is_constant()) { - out_ << (const_id.is_symbolic() ? " [symbolic" : " [template"); - if (const_id.inst_id() != inst_id) { - out_ << " = "; - FormatInstName(const_id.inst_id()); + !const_id.is_valid() || const_id.is_constant()) { + out_ << " ["; + if (const_id.is_valid()) { + out_ << (const_id.is_symbolic() ? "symbolic" : "template"); + if (const_id.inst_id() != inst_id) { + out_ << " = "; + FormatInstName(const_id.inst_id()); + } + } else { + out_ << const_id; } out_ << "]"; } out_ << "\n"; } + // Don't print a constant for ImportRefUnused. + auto FormatInstruction(InstId inst_id, ImportRefUnused inst) -> void { + Indent(); + FormatInstructionLHS(inst_id, inst); + out_ << ImportRefUnused::Kind.ir_name(); + FormatInstructionRHS(inst); + out_ << "\n"; + } + auto FormatInstructionLHS(InstId inst_id, Inst inst) -> void { switch (inst.kind().value_kind()) { case InstValueKind::Typed: diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index 6e8028483ea5..3f8ea4647f57 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -94,40 +94,52 @@ struct ConstantId : public IdBase, public Printable { // An ID for an expression whose phase cannot be determined because it // contains an error. This is always modeled as a template constant. static const ConstantId Error; + // An explicitly invalid ID. + static const ConstantId Invalid; // Returns the constant ID corresponding to a template constant, which should // either be in the `constants` block in the file or should be known to be // unique. static constexpr auto ForTemplateConstant(InstId const_id) -> ConstantId { - return ConstantId(const_id.index + 1); + return ConstantId(const_id.index + IndexOffset); } // Returns the constant ID corresponding to a symbolic constant, which should // either be in the `constants` block in the file or should be known to be // unique. static constexpr auto ForSymbolicConstant(InstId const_id) -> ConstantId { - // Avoid allocating index -1. - return ConstantId(-const_id.index - 1); + return ConstantId(-const_id.index - IndexOffset); } using IdBase::IdBase; - // Returns whether this represents a constant. - auto is_constant() const -> bool { return index != 0; } - // Returns whether this represents a symbolic constant. - auto is_symbolic() const -> bool { return index < 0; } - // Returns whether this represents a template constant. - auto is_template() const -> bool { return index > 0; } + // Returns whether this represents a constant. Requires is_valid. + auto is_constant() const -> bool { + CARBON_CHECK(is_valid()); + return *this != ConstantId::NotConstant; + } + // Returns whether this represents a symbolic constant. Requires is_valid. + auto is_symbolic() const -> bool { + CARBON_CHECK(is_valid()); + return index <= -IndexOffset; + } + // Returns whether this represents a template constant. Requires is_valid. + auto is_template() const -> bool { + CARBON_CHECK(is_valid()); + return index >= IndexOffset; + } // Returns the instruction that describes this constant value, or - // InstId::Invalid for a runtime value. - auto inst_id() const -> InstId { - static_assert(InstId::InvalidIndex == -1); - return InstId(std::abs(index) - 1); + // InstId::Invalid for a runtime value. Requires is_valid. + constexpr auto inst_id() const -> InstId { + CARBON_CHECK(is_valid()); + return InstId(Abs(index) - IndexOffset); } auto Print(llvm::raw_ostream& out) const -> void { - if (is_template()) { + if (!is_valid()) { + IdBase::Print(out); + } else if (is_template()) { out << "template " << inst_id(); } else if (is_symbolic()) { out << "symbolic " << inst_id(); @@ -137,13 +149,20 @@ struct ConstantId : public IdBase, public Printable { } private: - // ConstantIds don't have an invalid state. - using IdBase::is_valid; + // TODO: C++23 makes std::abs constexpr, but until then we mirror std::abs + // logic here. LLVM should still optimize this. + static constexpr auto Abs(int32_t i) -> int32_t { return i > 0 ? i : -i; } + + static constexpr int32_t NotConstantIndex = InvalidIndex - 1; + // The offset of InstId indices to ConstantId indices. + static constexpr int32_t IndexOffset = -NotConstantIndex + 1; }; -constexpr ConstantId ConstantId::NotConstant = ConstantId(0); +constexpr ConstantId ConstantId::NotConstant = ConstantId(NotConstantIndex); +static_assert(ConstantId::NotConstant.inst_id() == InstId::Invalid); constexpr ConstantId ConstantId::Error = ConstantId::ForTemplateConstant(InstId::BuiltinError); +constexpr ConstantId ConstantId::Invalid = ConstantId(InvalidIndex); // The ID of a bind name. struct BindNameId : public IdBase, public Printable { diff --git a/toolchain/sem_ir/value_stores.h b/toolchain/sem_ir/value_stores.h index 754528ee4c93..c59268802d14 100644 --- a/toolchain/sem_ir/value_stores.h +++ b/toolchain/sem_ir/value_stores.h @@ -119,12 +119,15 @@ class InstStore { // instructions. class ConstantValueStore { public: - // Returns the constant value of the requested instruction, or - // `ConstantId::NotConstant` if it is not constant. + explicit ConstantValueStore(ConstantId default_value) + : default_(default_value) {} + + // Returns the constant value of the requested instruction, which is default_ + // if unallocated. auto Get(InstId inst_id) const -> ConstantId { CARBON_CHECK(inst_id.index >= 0); return static_cast(inst_id.index) >= values_.size() - ? ConstantId::NotConstant + ? default_ : values_[inst_id.index]; } @@ -133,12 +136,14 @@ class ConstantValueStore { auto Set(InstId inst_id, ConstantId const_id) -> void { CARBON_CHECK(inst_id.index >= 0); if (static_cast(inst_id.index) >= values_.size()) { - values_.resize(inst_id.index + 1, ConstantId::NotConstant); + values_.resize(inst_id.index + 1, default_); } values_[inst_id.index] = const_id; } private: + const ConstantId default_; + // A mapping from `InstId::index` to the corresponding constant value. This is // expected to be sparse, and may be smaller than the list of instructions if // there are trailing non-constant instructions.