Skip to content

Commit

Permalink
Collection of minor tweaks to get approx. 10-15% compile time (carbon…
Browse files Browse the repository at this point in the history
…-language#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.
  • Loading branch information
chandlerc authored and dwblaikie committed Aug 23, 2024
1 parent c6ae098 commit 6c7297d
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 24 deletions.
7 changes: 4 additions & 3 deletions common/vlog.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions toolchain/base/value_store.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down
10 changes: 0 additions & 10 deletions toolchain/parse/tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,6 @@ auto Tree::postorder() const -> llvm::iterator_range<PostorderIterator> {
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;
Expand Down
10 changes: 8 additions & 2 deletions toolchain/parse/tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,16 @@ class Tree : public Printable<Tree> {

// 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;
Expand Down
4 changes: 2 additions & 2 deletions toolchain/sem_ir/constant.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<size_t>(inst_id.index) >= values_.size()
? default_
: values_[inst_id.index];
Expand All @@ -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<size_t>(inst_id.index) >= values_.size()) {
values_.resize(inst_id.index + 1, default_);
}
Expand Down
10 changes: 5 additions & 5 deletions toolchain/sem_ir/ids.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,17 @@ struct ConstantId : public IdBase, public Printable<ConstantId> {

// 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;
}

Expand Down Expand Up @@ -174,14 +174,14 @@ struct ConstantId : public IdBase, public Printable<ConstantId> {
// 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;
}

Expand Down

0 comments on commit 6c7297d

Please sign in to comment.