From 5d0ec91c20ed2074bd47685cb7a81d0a947bfa8f Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Fri, 23 Aug 2024 16:43:54 +0200 Subject: [PATCH] Collection of minor tweaks to get approx. 10-15% compile time (#4245) Most of these are about enabling inlining, in a couple of cases moving code to a header and throughout switching to `CARBON_DCHECK`. The code size of `CARBON_CHECK` seems to make inliing quite unreliable. I'm going to think about whether there are ways to improve this, but a reasonably small number of these seem worth switching for now to get some compile time savings. Also moves VLOG out of the hot path which helps a bit as well. All combined, this net a bit over 10%, although it varies a bit exactly how much. We're now pretty consistently over 800k lines/second for check in the compilation benchmark for files >=4k lines, which makes me happy. That's remarkably close to our original target. Not really planning to keep optimizing here, just was glancing at the profile and many of these stood out to me and were easy to fix. --- common/vlog.h | 7 ++++--- toolchain/base/value_store.h | 4 ++-- toolchain/parse/tree.cpp | 10 ---------- toolchain/parse/tree.h | 10 ++++++++-- toolchain/sem_ir/constant.h | 4 ++-- toolchain/sem_ir/ids.h | 10 +++++----- 6 files changed, 21 insertions(+), 24 deletions(-) diff --git a/common/vlog.h b/common/vlog.h index 859b157ce619..bb9fbe0bc7d8 100644 --- a/common/vlog.h +++ b/common/vlog.h @@ -13,9 +13,10 @@ namespace Carbon { // // For example: // CARBON_VLOG() << "Verbose message"; -#define CARBON_VLOG() \ - (vlog_stream_ == nullptr) ? (void)0 \ - : CARBON_VLOG_INTERNAL_STREAM(vlog_stream_) +#define CARBON_VLOG() \ + __builtin_expect(vlog_stream_ == nullptr, true) \ + ? (void)0 \ + : CARBON_VLOG_INTERNAL_STREAM(vlog_stream_) } // namespace Carbon diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h index 010362df7636..5c0a5c1e209e 100644 --- a/toolchain/base/value_store.h +++ b/toolchain/base/value_store.h @@ -165,13 +165,13 @@ class ValueStore // Returns a mutable value for an ID. auto Get(IdT id) -> RefType { - CARBON_CHECK(id.index >= 0) << id; + CARBON_DCHECK(id.index >= 0) << id; return values_[id.index]; } // Returns the value for an ID. auto Get(IdT id) const -> ConstRefType { - CARBON_CHECK(id.index >= 0) << id; + CARBON_DCHECK(id.index >= 0) << id; return values_[id.index]; } diff --git a/toolchain/parse/tree.cpp b/toolchain/parse/tree.cpp index 2edaf5141741..2e3a907edb4f 100644 --- a/toolchain/parse/tree.cpp +++ b/toolchain/parse/tree.cpp @@ -21,16 +21,6 @@ auto Tree::postorder() const -> llvm::iterator_range { PostorderIterator(NodeId(node_impls_.size()))); } -auto Tree::node_has_error(NodeId n) const -> bool { - CARBON_CHECK(n.is_valid()); - return node_impls_[n.index].has_error; -} - -auto Tree::node_kind(NodeId n) const -> NodeKind { - CARBON_CHECK(n.is_valid()); - return node_impls_[n.index].kind; -} - auto Tree::node_token(NodeId n) const -> Lex::TokenIndex { CARBON_CHECK(n.is_valid()); return node_impls_[n.index].token; diff --git a/toolchain/parse/tree.h b/toolchain/parse/tree.h index 699b25b29cdf..3815a6bcbfd5 100644 --- a/toolchain/parse/tree.h +++ b/toolchain/parse/tree.h @@ -115,10 +115,16 @@ class Tree : public Printable { // Tests whether a particular node contains an error and may not match the // full expected structure of the grammar. - auto node_has_error(NodeId n) const -> bool; + auto node_has_error(NodeId n) const -> bool { + CARBON_DCHECK(n.is_valid()); + return node_impls_[n.index].has_error; + } // Returns the kind of the given parse tree node. - auto node_kind(NodeId n) const -> NodeKind; + auto node_kind(NodeId n) const -> NodeKind { + CARBON_DCHECK(n.is_valid()); + return node_impls_[n.index].kind; + } // Returns the token the given parse tree node models. auto node_token(NodeId n) const -> Lex::TokenIndex; diff --git a/toolchain/sem_ir/constant.h b/toolchain/sem_ir/constant.h index 1684403e9f3d..a7fef04e50b7 100644 --- a/toolchain/sem_ir/constant.h +++ b/toolchain/sem_ir/constant.h @@ -41,7 +41,7 @@ class ConstantValueStore { // 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); + CARBON_DCHECK(inst_id.index >= 0); return static_cast(inst_id.index) >= values_.size() ? default_ : values_[inst_id.index]; @@ -50,7 +50,7 @@ class ConstantValueStore { // Sets the constant value of the given instruction, or sets that it is known // to not be a constant. auto Set(InstId inst_id, ConstantId const_id) -> void { - CARBON_CHECK(inst_id.index >= 0); + CARBON_DCHECK(inst_id.index >= 0); if (static_cast(inst_id.index) >= values_.size()) { values_.resize(inst_id.index + 1, default_); } diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index be9d5c912214..daf0405e8127 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -127,17 +127,17 @@ struct ConstantId : public IdBase, public Printable { // Returns whether this represents a constant. Requires is_valid. auto is_constant() const -> bool { - CARBON_CHECK(is_valid()); + CARBON_DCHECK(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()); + CARBON_DCHECK(is_valid()); return index <= FirstSymbolicIndex; } // Returns whether this represents a template constant. Requires is_valid. auto is_template() const -> bool { - CARBON_CHECK(is_valid()); + CARBON_DCHECK(is_valid()); return index >= 0; } @@ -174,14 +174,14 @@ struct ConstantId : public IdBase, public Printable { // Requires `is_template()`. Use `ConstantValueStore::GetInstId` to get the // instruction ID of a `ConstantId`. constexpr auto template_inst_id() const -> InstId { - CARBON_CHECK(is_template()); + CARBON_DCHECK(is_template()); return InstId(index); } // Returns the symbolic constant index that describes this symbolic constant // value. Requires `is_symbolic()`. constexpr auto symbolic_index() const -> int32_t { - CARBON_CHECK(is_symbolic()); + CARBON_DCHECK(is_symbolic()); return FirstSymbolicIndex - index; }