From 04e4f7b63d483bdc0d8fe4b9cd236a8c5766e4d6 Mon Sep 17 00:00:00 2001 From: Yu-Sheng Lin Date: Mon, 23 Mar 2026 02:02:24 +0800 Subject: [PATCH] Update according review comments --- docs/guide/install.rst | 4 ++-- include/fstcpp/fstcpp_writer.cpp | 25 ++++++++++++------------ include/fstcpp/fstcpp_writer.h | 3 +++ include/verilated.mk.in | 1 - include/verilated_fst_c.cpp | 33 ++++++++++---------------------- include/verilated_fst_c.h | 10 +++++----- 6 files changed, 32 insertions(+), 44 deletions(-) diff --git a/docs/guide/install.rst b/docs/guide/install.rst index 8ebb45110..f25ac2dec 100644 --- a/docs/guide/install.rst +++ b/docs/guide/install.rst @@ -64,7 +64,7 @@ In brief, to install from git: #sudo apt-get install libgoogle-perftools-dev libjemalloc-dev numactl perl-doc #sudo apt-get install libfl2 # Ubuntu only (ignore if gives error) #sudo apt-get install libfl-dev # Ubuntu only (ignore if gives error) - #sudo apt-get install zlibc zlib1g zlib1g-dev # Ubuntu only (ignore if gives error) + #sudo apt-get install zlibc zlib1g zlib1g-dev liblz4 liblz4-dev # Ubuntu only (ignore if gives error) git clone https://github.com/verilator/verilator # Only first time @@ -116,7 +116,7 @@ To build or run Verilator, you need these standard packages: sudo apt-get install libgz # Non-Ubuntu (ignore if gives error) sudo apt-get install libfl2 # Ubuntu only (ignore if gives error) sudo apt-get install libfl-dev # Ubuntu only (ignore if gives error) - sudo apt-get install zlibc zlib1g zlib1g-dev # Ubuntu only (ignore if gives error) + sudo apt-get install zlibc zlib1g zlib1g-dev liblz4 liblz4-dev # Ubuntu only (ignore if gives error) For SystemC: diff --git a/include/fstcpp/fstcpp_writer.cpp b/include/fstcpp/fstcpp_writer.cpp index 7f7b46458..2756e23b9 100644 --- a/include/fstcpp/fstcpp_writer.cpp +++ b/include/fstcpp/fstcpp_writer.cpp @@ -23,13 +23,13 @@ #include "fstcpp/fstcpp_stream_write_helper.h" #include "fstcpp/fstcpp_variable_info.h" -// AT(x) is used to access vector at index x, and it will throw exception if out of bound +// AT(vec, x) is used to access vector at index x, and it will throw exception if out of bound // in debug mode, but in release mode, it will not throw exception -// Usually you should only need AT(x) only at very hot code path. +// Usually you should only need AT(vec, x) only at very hot code path. #ifndef NDEBUG -# define AT(x) .at(x) +# define AT(vec, x) (vec.at(x)) #else -# define AT(x) [x] +# define AT(vec, x) (vec[x]) #endif namespace fst { @@ -228,7 +228,7 @@ void Writer::emitTimeChange(uint64_t tim) { template void Writer::emitValueChangeHelper_(Handle handle, T &&...val) { // Let data prefetch go first - VariableInfo &var_info = m_value_change_data_.m_variable_infos AT(handle - 1); + VariableInfo &var_info = AT(m_value_change_data_.m_variable_infos, handle - 1); #if defined(__GNUC__) || defined(__clang__) __builtin_prefetch(var_info.data_ptr() + var_info.size() - 1, 1, 0); #endif @@ -256,7 +256,7 @@ void Writer::emitValueChange(Handle handle, uint64_t val) { void Writer::emitValueChange(Handle handle, const char *val) { finalizeHierarchy_(); - VariableInfo &var_info = m_value_change_data_.m_variable_infos AT(handle - 1); + VariableInfo &var_info = AT(m_value_change_data_.m_variable_infos, handle - 1); // For double handles, const char* is interpreted as a double* (8B) // This double shall be written out as raw IEEE 754 double @@ -271,24 +271,23 @@ void Writer::emitValueChange(Handle handle, const char *val) { FST_DCHECK_NE(bitwidth, 0); val += bitwidth; - static std::vector t_packed_value_buffer; const unsigned num_words{(bitwidth + 63) / 64}; - t_packed_value_buffer.assign(num_words, 0); + m_packed_value_buffer_.assign(num_words, 0); for (unsigned i = 0; i < num_words; ++i) { const char *start{val - std::min((i + 1) * 64, bitwidth)}; const char *end{val - 64 * i}; - t_packed_value_buffer[i] = 0; + m_packed_value_buffer_[i] = 0; for (const char *p = start; p < end; ++p) { // No checking for invalid characters, follow original C implementation - t_packed_value_buffer[i] <<= 1; - t_packed_value_buffer[i] |= static_cast(*p - '0'); + m_packed_value_buffer_[i] <<= 1; + m_packed_value_buffer_[i] |= static_cast(*p - '0'); } } if (bitwidth <= 64) { - emitValueChange(handle, t_packed_value_buffer.front()); + emitValueChange(handle, m_packed_value_buffer_.front()); } else { - emitValueChange(handle, t_packed_value_buffer.data(), EncodingType::BINARY); + emitValueChange(handle, m_packed_value_buffer_.data(), EncodingType::BINARY); } } diff --git a/include/fstcpp/fstcpp_writer.h b/include/fstcpp/fstcpp_writer.h index e50a74214..532fdff93 100644 --- a/include/fstcpp/fstcpp_writer.h +++ b/include/fstcpp/fstcpp_writer.h @@ -77,6 +77,9 @@ private: std::ofstream m_main_fst_file_{}; std::vector m_hierarchy_buffer_{}; std::vector m_geometry_buffer_{}; + // Temporary buffer for packing bit strings into words + // Only used in emitValueChange(Handle, const char*) + std::vector m_packed_value_buffer_{}; Header m_header_{}; detail::BlackoutData m_blackout_data_{}; // Not implemented yet detail::ValueChangeData m_value_change_data_{}; diff --git a/include/verilated.mk.in b/include/verilated.mk.in index e517e9161..3ae635175 100644 --- a/include/verilated.mk.in +++ b/include/verilated.mk.in @@ -104,7 +104,6 @@ CPPFLAGS += -I. $(VK_CPPFLAGS_WALL) $(VK_CPPFLAGS_ALWAYS) VPATH += .. VPATH += $(VERILATOR_ROOT)/include VPATH += $(VERILATOR_ROOT)/include/vltstd -VPATH += $(VERILATOR_ROOT)/include/fstcpp LDFLAGS += $(CFG_LDFLAGS_VERILATED) diff --git a/include/verilated_fst_c.cpp b/include/verilated_fst_c.cpp index f3ac9d3e1..61fcd4897 100644 --- a/include/verilated_fst_c.cpp +++ b/include/verilated_fst_c.cpp @@ -395,61 +395,48 @@ VL_ATTR_ALWINLINE void VerilatedFstBuffer::emitBit(uint32_t code, CData newval) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); - m_fst->emitValueChange(m_symbolp[code], newval ? 1 : 0); + m_fst->emitValueChange(m_symbolp[code], uint64_t(newval)); } VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitCData(uint32_t code, CData newval, int bits) { +void VerilatedFstBuffer::emitCData(uint32_t code, CData newval, int) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); m_fst->emitValueChange(m_symbolp[code], newval); } VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitSData(uint32_t code, SData newval, int bits) { +void VerilatedFstBuffer::emitSData(uint32_t code, SData newval, int) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); m_fst->emitValueChange(m_symbolp[code], newval); } VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitIData(uint32_t code, IData newval, int bits) { +void VerilatedFstBuffer::emitIData(uint32_t code, IData newval, int) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); m_fst->emitValueChange(m_symbolp[code], newval); } VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitQData(uint32_t code, QData newval, int bits) { +void VerilatedFstBuffer::emitQData(uint32_t code, QData newval, int) { VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); m_fst->emitValueChange(m_symbolp[code], newval); } VL_ATTR_ALWINLINE -void VerilatedFstBuffer::emitWData(uint32_t code, const WData* newvalp, int bits) { - // While emitValueChange has a uint32_t* version - // It does the same conversion, allocating a pointer and copying the data - // So I decide to use the verilator buffer directly. - // The buffer were designed to hold maxBits() char, - // so it is very safe to use it as uint64_t*. - int words = VL_WORDS_I(bits); - uint64_t* wp = reinterpret_cast(m_strbufp); - // cast newvalp (uint32_t[words]) to wp (uint64_t[ceil(words/2)]) - for (int i = 0; i < words/2; ++i) { - wp[i] = newvalp[i*2+1]; - wp[i] <<= 32; - wp[i] |= newvalp[i*2]; - } - if (words % 2 == 1) { - wp[words/2] = newvalp[words-1]; - } +void VerilatedFstBuffer::emitWData(uint32_t code, const WData* newvalp, int) { + VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); - m_fst->emitValueChange(m_symbolp[code], wp); + // call emitValueChange(handle, uint32_t*) + m_fst->emitValueChange(m_symbolp[code], newvalp); } VL_ATTR_ALWINLINE void VerilatedFstBuffer::emitDouble(uint32_t code, double newval) { + VL_DEBUG_IFDEF(assert(m_symbolp[code]);); m_owner.emitTimeChangeMaybe(); uint64_t newval_u64; std::memcpy(&newval_u64, &newval, sizeof(newval_u64)); diff --git a/include/verilated_fst_c.h b/include/verilated_fst_c.h index 387244d37..008698364 100644 --- a/include/verilated_fst_c.h +++ b/include/verilated_fst_c.h @@ -231,11 +231,11 @@ class VerilatedFstBuffer VL_NOT_FINAL { // called from only one place (the full* methods), so always inline them. VL_ATTR_ALWINLINE void emitEvent(uint32_t code); VL_ATTR_ALWINLINE void emitBit(uint32_t code, CData newval); - VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int bits); - VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int bits); - VL_ATTR_ALWINLINE void emitIData(uint32_t code, IData newval, int bits); - VL_ATTR_ALWINLINE void emitQData(uint32_t code, QData newval, int bits); - VL_ATTR_ALWINLINE void emitWData(uint32_t code, const WData* newvalp, int bits); + VL_ATTR_ALWINLINE void emitCData(uint32_t code, CData newval, int); + VL_ATTR_ALWINLINE void emitSData(uint32_t code, SData newval, int); + VL_ATTR_ALWINLINE void emitIData(uint32_t code, IData newval, int); + VL_ATTR_ALWINLINE void emitQData(uint32_t code, QData newval, int); + VL_ATTR_ALWINLINE void emitWData(uint32_t code, const WData* newvalp, int); VL_ATTR_ALWINLINE void emitDouble(uint32_t code, double newval); };