Add an allocator to store the symbol names.
This CL adds an allocator, owned by the SymbolTable, which stores the
names of all the symbols in the table. The Symbols then have a
`string_view` to their name.
Change-Id: I28e5b2aefcf9f67c1877b7ebab52416f780bd8c6
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/127300
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3eb8136..2d44a64 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -172,7 +172,6 @@
option_if_not_defined(TINT_ENABLE_BREAK_IN_DEBUGGER "Enable tint::debugger::Break()" OFF)
option_if_not_defined(TINT_CHECK_CHROMIUM_STYLE "Check for [chromium-style] issues during build" OFF)
-option_if_not_defined(TINT_SYMBOL_STORE_DEBUG_NAME "Enable storing of name in tint::ast::Symbol to help debugging the AST" OFF)
option_if_not_defined(TINT_RANDOMIZE_HASHES "Randomize the hash seed value to detect non-deterministic output" OFF)
# Recommended setting for compability with future abseil releases.
diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt
index 6c7acfb..8ac5b15 100644
--- a/src/tint/CMakeLists.txt
+++ b/src/tint/CMakeLists.txt
@@ -786,9 +786,6 @@
add_library(libtint ${TINT_LIB_SRCS})
tint_default_compile_options(libtint)
target_link_libraries(libtint tint_diagnostic_utils absl_strings)
-if (${TINT_SYMBOL_STORE_DEBUG_NAME})
- target_compile_definitions(libtint PUBLIC "TINT_SYMBOL_STORE_DEBUG_NAME=1")
-endif()
set_target_properties(libtint PROPERTIES OUTPUT_NAME "tint")
if (${TINT_BUILD_FUZZERS})
diff --git a/src/tint/ast/identifier_expression_test.cc b/src/tint/ast/identifier_expression_test.cc
index 0e0e7c0..6e9b367 100644
--- a/src/tint/ast/identifier_expression_test.cc
+++ b/src/tint/ast/identifier_expression_test.cc
@@ -24,12 +24,12 @@
TEST_F(IdentifierExpressionTest, Creation) {
auto* i = Expr("ident");
- EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+ EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
}
TEST_F(IdentifierExpressionTest, CreationTemplated) {
auto* i = Expr(Ident("ident", true));
- EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+ EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
auto* tmpl_ident = i->identifier->As<TemplatedIdentifier>();
ASSERT_NE(tmpl_ident, nullptr);
EXPECT_EQ(tmpl_ident->arguments.Length(), 1_u);
@@ -38,7 +38,7 @@
TEST_F(IdentifierExpressionTest, Creation_WithSource) {
auto* i = Expr(Source{{20, 2}}, "ident");
- EXPECT_EQ(i->identifier->symbol, Symbol(1, ID()));
+ EXPECT_EQ(i->identifier->symbol, Symbol(1, ID(), "ident"));
EXPECT_EQ(i->source.range, (Source::Range{{20, 2}}));
EXPECT_EQ(i->identifier->source.range, (Source::Range{{20, 2}}));
diff --git a/src/tint/ast/identifier_test.cc b/src/tint/ast/identifier_test.cc
index 2d9d48f..9b0b4f4 100644
--- a/src/tint/ast/identifier_test.cc
+++ b/src/tint/ast/identifier_test.cc
@@ -22,12 +22,12 @@
TEST_F(IdentifierTest, Creation) {
auto* i = Ident("ident");
- EXPECT_EQ(i->symbol, Symbol(1, ID()));
+ EXPECT_EQ(i->symbol, Symbol(1, ID(), "ident"));
}
TEST_F(IdentifierTest, Creation_WithSource) {
auto* i = Ident(Source{{20, 2}}, "ident");
- EXPECT_EQ(i->symbol, Symbol(1, ID()));
+ EXPECT_EQ(i->symbol, Symbol(1, ID(), "ident"));
auto src = i->source;
EXPECT_EQ(src.range.begin.line, 20u);
diff --git a/src/tint/clone_context.cc b/src/tint/clone_context.cc
index 83e6d6c..3590301 100644
--- a/src/tint/clone_context.cc
+++ b/src/tint/clone_context.cc
@@ -33,7 +33,7 @@
// Almost all transforms will want to clone all symbols before doing any
// work, to avoid any newly created symbols clashing with existing symbols
// in the source program and causing them to be renamed.
- from->Symbols().Foreach([&](Symbol s, const std::string&) { Clone(s); });
+ from->Symbols().Foreach([&](Symbol s) { Clone(s); });
}
}
diff --git a/src/tint/program_builder.cc b/src/tint/program_builder.cc
index 99fce9e..bfb7edb 100644
--- a/src/tint/program_builder.cc
+++ b/src/tint/program_builder.cc
@@ -76,7 +76,7 @@
builder.ast_ =
builder.create<ast::Module>(program->AST().source, program->AST().GlobalDeclarations());
builder.sem_ = sem::Info::Wrap(program->Sem());
- builder.symbols_ = program->Symbols();
+ builder.symbols_.Wrap(program->Symbols());
builder.diagnostics_ = program->Diagnostics();
return builder;
}
diff --git a/src/tint/reader/spirv/parser_type_test.cc b/src/tint/reader/spirv/parser_type_test.cc
index 2dd3cca..f864818 100644
--- a/src/tint/reader/spirv/parser_type_test.cc
+++ b/src/tint/reader/spirv/parser_type_test.cc
@@ -21,7 +21,7 @@
namespace {
TEST(SpvParserTypeTest, SameArgumentsGivesSamePointer) {
- Symbol sym(Symbol(1, {}));
+ Symbol sym(Symbol(1, {}, "1"));
TypeManager ty;
EXPECT_EQ(ty.Void(), ty.Void());
@@ -50,8 +50,8 @@
}
TEST(SpvParserTypeTest, DifferentArgumentsGivesDifferentPointer) {
- Symbol sym_a(Symbol(1, {}));
- Symbol sym_b(Symbol(2, {}));
+ Symbol sym_a(Symbol(1, {}, "1"));
+ Symbol sym_b(Symbol(2, {}, "2"));
TypeManager ty;
EXPECT_NE(ty.Pointer(ty.I32(), builtin::AddressSpace::kUndefined),
diff --git a/src/tint/scope_stack_test.cc b/src/tint/scope_stack_test.cc
index aeb7e73..f899e51 100644
--- a/src/tint/scope_stack_test.cc
+++ b/src/tint/scope_stack_test.cc
@@ -23,8 +23,8 @@
TEST_F(ScopeStackTest, Get) {
ScopeStack<Symbol, uint32_t> s;
- Symbol a(1, ID());
- Symbol b(3, ID());
+ Symbol a(1, ID(), "1");
+ Symbol b(3, ID(), "3");
s.Push();
s.Set(a, 5u);
s.Set(b, 10u);
@@ -45,14 +45,14 @@
TEST_F(ScopeStackTest, Get_MissingSymbol) {
ScopeStack<Symbol, uint32_t> s;
- Symbol sym(1, ID());
+ Symbol sym(1, ID(), "1");
EXPECT_EQ(s.Get(sym), 0u);
}
TEST_F(ScopeStackTest, Set) {
ScopeStack<Symbol, uint32_t> s;
- Symbol a(1, ID());
- Symbol b(2, ID());
+ Symbol a(1, ID(), "1");
+ Symbol b(2, ID(), "2");
EXPECT_EQ(s.Set(a, 5u), 0u);
EXPECT_EQ(s.Get(a), 5u);
@@ -69,8 +69,8 @@
TEST_F(ScopeStackTest, Clear) {
ScopeStack<Symbol, uint32_t> s;
- Symbol a(1, ID());
- Symbol b(2, ID());
+ Symbol a(1, ID(), "1");
+ Symbol b(2, ID(), "2");
EXPECT_EQ(s.Set(a, 5u), 0u);
EXPECT_EQ(s.Get(a), 5u);
diff --git a/src/tint/symbol.cc b/src/tint/symbol.cc
index 32177f6..b64947f 100644
--- a/src/tint/symbol.cc
+++ b/src/tint/symbol.cc
@@ -20,12 +20,8 @@
Symbol::Symbol() = default;
-Symbol::Symbol(uint32_t val, tint::ProgramID program_id) : val_(val), program_id_(program_id) {}
-
-#if TINT_SYMBOL_STORE_DEBUG_NAME
-Symbol::Symbol(uint32_t val, tint::ProgramID pid, std::string debug_name)
- : val_(val), program_id_(pid), debug_name_(std::move(debug_name)) {}
-#endif
+Symbol::Symbol(uint32_t val, tint::ProgramID pid, std::string_view name)
+ : val_(val), program_id_(pid), name_(name) {}
Symbol::Symbol(const Symbol& o) = default;
@@ -56,4 +52,8 @@
return "$" + std::to_string(val_);
}
+std::string Symbol::Name() const {
+ return std::string(name_);
+}
+
} // namespace tint
diff --git a/src/tint/symbol.h b/src/tint/symbol.h
index c093673..a31388d 100644
--- a/src/tint/symbol.h
+++ b/src/tint/symbol.h
@@ -15,13 +15,6 @@
#ifndef SRC_TINT_SYMBOL_H_
#define SRC_TINT_SYMBOL_H_
-// If TINT_SYMBOL_STORE_DEBUG_NAME is 1, Symbol instances store a `debug_name_`
-// member initialized with the name of the identifier they represent. This
-// member is not exposed, but is useful for debugging purposes.
-#ifndef TINT_SYMBOL_STORE_DEBUG_NAME
-#define TINT_SYMBOL_STORE_DEBUG_NAME 0
-#endif
-
#include <string>
#include "src/tint/program_id.h"
@@ -36,15 +29,10 @@
Symbol();
/// Constructor
/// @param val the symbol value
- /// @param program_id the identifier of the program that owns this Symbol
- Symbol(uint32_t val, tint::ProgramID program_id);
-#if TINT_SYMBOL_STORE_DEBUG_NAME
- /// Constructor
- /// @param val the symbol value
/// @param pid the identifier of the program that owns this Symbol
- /// @param debug_name name of symbols used only for debugging
- Symbol(uint32_t val, tint::ProgramID pid, std::string debug_name);
-#endif
+ /// @param name the name this symbol represents
+ Symbol(uint32_t val, tint::ProgramID pid, std::string_view name);
+
/// Copy constructor
/// @param o the symbol to copy
Symbol(const Symbol& o);
@@ -88,15 +76,17 @@
/// @return the string representation of the symbol
std::string to_str() const;
+ /// Converts the symbol to the registered name
+ /// @returns the string representing the name of the symbol
+ std::string Name() const;
+
/// @returns the identifier of the Program that owns this symbol.
tint::ProgramID ProgramID() const { return program_id_; }
private:
uint32_t val_ = static_cast<uint32_t>(-1);
tint::ProgramID program_id_;
-#if TINT_SYMBOL_STORE_DEBUG_NAME
- std::string debug_name_;
-#endif
+ std::string_view name_;
};
/// @param sym the Symbol
diff --git a/src/tint/symbol_table.cc b/src/tint/symbol_table.cc
index 4f21d0b..c82a284 100644
--- a/src/tint/symbol_table.cc
+++ b/src/tint/symbol_table.cc
@@ -20,14 +20,10 @@
SymbolTable::SymbolTable(tint::ProgramID program_id) : program_id_(program_id) {}
-SymbolTable::SymbolTable(const SymbolTable&) = default;
-
SymbolTable::SymbolTable(SymbolTable&&) = default;
SymbolTable::~SymbolTable() = default;
-SymbolTable& SymbolTable::operator=(const SymbolTable& other) = default;
-
SymbolTable& SymbolTable::operator=(SymbolTable&&) = default;
Symbol SymbolTable::Register(const std::string& name) {
@@ -38,15 +34,19 @@
return *it;
}
-#if TINT_SYMBOL_STORE_DEBUG_NAME
- Symbol sym(next_symbol_, program_id_, name);
-#else
- Symbol sym(next_symbol_, program_id_);
-#endif
+ char* name_mem = name_allocator_.Allocate(name.length() + 1);
+ if (name_mem == nullptr) {
+ return Symbol();
+ }
+
+ memcpy(name_mem, name.data(), name.length() + 1);
+ std::string_view nv(name_mem, name.length());
+
+ Symbol sym(next_symbol_, program_id_, nv);
+
++next_symbol_;
- name_to_symbol_.Add(name, sym);
- symbol_to_name_.Add(sym, name);
+ name_to_symbol_.Add(name_mem, sym);
return sym;
}
@@ -58,12 +58,7 @@
std::string SymbolTable::NameFor(const Symbol symbol) const {
TINT_ASSERT_PROGRAM_IDS_EQUAL(Symbol, program_id_, symbol);
- auto it = symbol_to_name_.Find(symbol);
- if (!it) {
- return symbol.to_str();
- }
-
- return *it;
+ return symbol.Name();
}
Symbol SymbolTable::New(std::string prefix /* = "" */) {
diff --git a/src/tint/symbol_table.h b/src/tint/symbol_table.h
index 937a764..ea9199e 100644
--- a/src/tint/symbol_table.h
+++ b/src/tint/symbol_table.h
@@ -16,9 +16,10 @@
#define SRC_TINT_SYMBOL_TABLE_H_
#include <string>
-#include "utils/hashmap.h"
#include "src/tint/symbol.h"
+#include "utils/bump_allocator.h"
+#include "utils/hashmap.h"
namespace tint {
@@ -29,22 +30,29 @@
/// @param program_id the identifier of the program that owns this symbol
/// table
explicit SymbolTable(tint::ProgramID program_id);
- /// Copy constructor
- SymbolTable(const SymbolTable&);
/// Move Constructor
SymbolTable(SymbolTable&&);
/// Destructor
~SymbolTable();
- /// Copy assignment
- /// @param other the symbol table to copy
- /// @returns the new symbol table
- SymbolTable& operator=(const SymbolTable& other);
/// Move assignment
/// @param other the symbol table to move
/// @returns the symbol table
SymbolTable& operator=(SymbolTable&& other);
+ /// Wrap sets this symbol table to hold symbols which point to the allocated names in @p o.
+ /// The symbol table after Wrap is intended to temporarily extend the objects
+ /// of an existing immutable SymbolTable
+ /// As the copied objects are owned by @p o, @p o must not be destructed
+ /// or assigned while using this symbol table.
+ /// @param o the immutable SymbolTable to extend
+ void Wrap(const SymbolTable& o) {
+ next_symbol_ = o.next_symbol_;
+ name_to_symbol_ = o.name_to_symbol_;
+ last_prefix_to_index_ = o.last_prefix_to_index_;
+ program_id_ = o.program_id_;
+ }
+
/// Registers a name into the symbol table, returning the Symbol.
/// @param name the name to register
/// @returns the symbol representing the given name
@@ -70,11 +78,11 @@
/// Foreach calls the callback function `F` for each symbol in the table.
/// @param callback must be a function or function-like object with the
- /// signature: `void(Symbol, const std::string&)`
+ /// signature: `void(Symbol)`
template <typename F>
void Foreach(F&& callback) const {
- for (auto it : symbol_to_name_) {
- callback(it.key, it.value);
+ for (auto it : name_to_symbol_) {
+ callback(it.value);
}
}
@@ -82,13 +90,17 @@
tint::ProgramID ProgramID() const { return program_id_; }
private:
+ SymbolTable(const SymbolTable&) = delete;
+ SymbolTable& operator=(const SymbolTable& other) = delete;
+
// The value to be associated to the next registered symbol table entry.
uint32_t next_symbol_ = 1;
- utils::Hashmap<Symbol, std::string, 0> symbol_to_name_;
- utils::Hashmap<std::string, Symbol, 0> name_to_symbol_;
+ utils::Hashmap<std::string_view, Symbol, 0> name_to_symbol_;
utils::Hashmap<std::string, size_t, 0> last_prefix_to_index_;
tint::ProgramID program_id_;
+
+ utils::BumpAllocator name_allocator_;
};
/// @param symbol_table the SymbolTable
diff --git a/src/tint/symbol_table_test.cc b/src/tint/symbol_table_test.cc
index 1cf6d1a..28f664d 100644
--- a/src/tint/symbol_table_test.cc
+++ b/src/tint/symbol_table_test.cc
@@ -24,16 +24,16 @@
TEST_F(SymbolTableTest, GeneratesSymbolForName) {
auto program_id = ProgramID::New();
SymbolTable s{program_id};
- EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
- EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
+ EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
+ EXPECT_EQ(Symbol(2, program_id, "another_name"), s.Register("another_name"));
}
TEST_F(SymbolTableTest, DeduplicatesNames) {
auto program_id = ProgramID::New();
SymbolTable s{program_id};
- EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
- EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
- EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+ EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
+ EXPECT_EQ(Symbol(2, program_id, "another_name"), s.Register("another_name"));
+ EXPECT_EQ(Symbol(1, program_id, "name"), s.Register("name"));
}
TEST_F(SymbolTableTest, ReturnsNameForSymbol) {
@@ -46,7 +46,7 @@
TEST_F(SymbolTableTest, ReturnsBlankForMissingSymbol) {
auto program_id = ProgramID::New();
SymbolTable s{program_id};
- EXPECT_EQ("$2", s.NameFor(Symbol(2, program_id)));
+ EXPECT_EQ("$2", s.NameFor(Symbol(2, program_id, "$2")));
}
TEST_F(SymbolTableTest, AssertsForBlankString) {
diff --git a/src/tint/symbol_test.cc b/src/tint/symbol_test.cc
index db8e321..6285414 100644
--- a/src/tint/symbol_test.cc
+++ b/src/tint/symbol_test.cc
@@ -22,12 +22,12 @@
using SymbolTest = testing::Test;
TEST_F(SymbolTest, ToStr) {
- Symbol sym(1, ProgramID::New());
+ Symbol sym(1, ProgramID::New(), "");
EXPECT_EQ("$1", sym.to_str());
}
TEST_F(SymbolTest, CopyAssign) {
- Symbol sym1(1, ProgramID::New());
+ Symbol sym1(1, ProgramID::New(), "");
Symbol sym2;
EXPECT_FALSE(sym2.IsValid());
@@ -38,9 +38,9 @@
TEST_F(SymbolTest, Comparison) {
auto program_id = ProgramID::New();
- Symbol sym1(1, program_id);
- Symbol sym2(2, program_id);
- Symbol sym3(1, program_id);
+ Symbol sym1(1, program_id, "1");
+ Symbol sym2(2, program_id, "2");
+ Symbol sym3(1, program_id, "3");
EXPECT_TRUE(sym1 == sym3);
EXPECT_FALSE(sym1 != sym3);
diff --git a/src/tint/tint.natvis b/src/tint/tint.natvis
index 062bca7..6096d84 100644
--- a/src/tint/tint.natvis
+++ b/src/tint/tint.natvis
@@ -55,8 +55,7 @@
</Type>
<Type Name="tint::Symbol">
- <!-- Requires TINT_SYMBOL_STORE_DEBUG_NAME defined to 1 -->
- <DisplayString Optional="true">{debug_name_,sb}</DisplayString>
+ <DisplayString Optional="true">{name_,sb}</DisplayString>
</Type>
<!--=================================================================-->
