From ed81cc5f8106134e08c9aeaca74d86839945fd57 Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 09:22:39 +0000 Subject: [PATCH 1/6] =?UTF-8?q?cxxrtl:=20rename=20observer::{on=5Fcommit?= =?UTF-8?q?=E2=86=92on=5Fupdate}.=20(breaking=20change)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name `on_commit` was terrible since it would not be called, as one may conclude from the name, on each `commit()`, but only whenever that method actually updates a value. --- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 14 +++++++------- backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index 3f8247226..e2729bfe4 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -865,19 +865,19 @@ struct observer { // Called when the `commit()` method for a wire is about to update the `chunks` chunks at `base` with `chunks` chunks // at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to the wire chunk count and // `base` points to the first chunk. - virtual void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value) = 0; + virtual void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) = 0; // Called when the `commit()` method for a memory is about to update the `chunks` chunks at `&base[chunks * index]` // with `chunks` chunks at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to // the memory element chunk count and `base` points to the first chunk of the first element of the memory. - virtual void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) = 0; + virtual void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) = 0; }; // The `null_observer` class has the same interface as `observer`, but has no invocation overhead, since its methods // are final and have no implementation. This allows the observer feature to be zero-cost when not in use. struct null_observer final: observer { - void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value) override {} - void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override {} + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) override {} + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override {} }; template @@ -916,12 +916,12 @@ struct wire { // This method intentionally takes a mandatory argument (to make it more difficult to misuse in // black box implementations, leading to missed observer events). It is generic over its argument - // to make sure the `on_commit` call is devirtualized. This is somewhat awkward but lets us keep + // to make sure the `on_update` call is devirtualized. This is somewhat awkward but lets us keep // a single implementation for both this method and the one in `memory`. template bool commit(ObserverT &observer) { if (curr != next) { - observer.on_commit(curr.chunks, curr.data, next.data); + observer.on_update(curr.chunks, curr.data, next.data); curr = next; return true; } @@ -1003,7 +1003,7 @@ struct memory { value elem = data[entry.index]; elem = elem.update(entry.val, entry.mask); if (data[entry.index] != elem) { - observer.on_commit(value::chunks, data[0].data, elem.data, entry.index); + observer.on_update(value::chunks, data[0].data, elem.data, entry.index); changed |= true; } data[entry.index] = elem; diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h index 94f59bb0d..e44b1c4e1 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h @@ -561,12 +561,12 @@ public: spool::writer *writer; CXXRTL_ALWAYS_INLINE - void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value) override { + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) override { writer->write_change(ident_lookup->at(base), chunks, value); } CXXRTL_ALWAYS_INLINE - void on_commit(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override { + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override { writer->write_change(ident_lookup->at(base), chunks, value, index); } } record_observer; From bf1a99da09b2db6eb8a44e324de48f8e4092e9a1 Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 09:53:43 +0000 Subject: [PATCH 2/6] cxxrtl: make observer methods non-virtual. (breaking change) This avoids having to devirtualize them later to get performance back, and simplifies the code a bit. The change is prompted by the desire to add a similar observer object to `eval()`, and a repeated consideration of whether the dispatch on these should be virtual in first place. --- backends/cxxrtl/cxxrtl_backend.cc | 4 ++-- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 14 +++----------- backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h | 10 ++++------ 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index ff11d7fe0..30995ea11 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -2391,7 +2391,7 @@ struct CxxrtlWorker { f << indent << "}\n"; f << "\n"; f << indent << "bool commit() override {\n"; - f << indent << indent << "null_observer observer;\n"; + f << indent << indent << "observer observer;\n"; f << indent << indent << "return commit<>(observer);\n"; f << indent << "}\n"; if (debug_info) { @@ -2486,7 +2486,7 @@ struct CxxrtlWorker { f << indent << "}\n"; f << "\n"; f << indent << "bool commit() override {\n"; - f << indent << indent << "null_observer observer;\n"; + f << indent << indent << "observer observer;\n"; f << indent << indent << "return commit<>(observer);\n"; f << indent << "}\n"; if (debug_info) { diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index e2729bfe4..bd336f481 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -865,19 +865,12 @@ struct observer { // Called when the `commit()` method for a wire is about to update the `chunks` chunks at `base` with `chunks` chunks // at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to the wire chunk count and // `base` points to the first chunk. - virtual void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) = 0; + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) {} // Called when the `commit()` method for a memory is about to update the `chunks` chunks at `&base[chunks * index]` // with `chunks` chunks at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to // the memory element chunk count and `base` points to the first chunk of the first element of the memory. - virtual void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) = 0; -}; - -// The `null_observer` class has the same interface as `observer`, but has no invocation overhead, since its methods -// are final and have no implementation. This allows the observer feature to be zero-cost when not in use. -struct null_observer final: observer { - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) override {} - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override {} + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) {} }; template @@ -916,8 +909,7 @@ struct wire { // This method intentionally takes a mandatory argument (to make it more difficult to misuse in // black box implementations, leading to missed observer events). It is generic over its argument - // to make sure the `on_update` call is devirtualized. This is somewhat awkward but lets us keep - // a single implementation for both this method and the one in `memory`. + // to allow the `on_update` method to be non-virtual. template bool commit(ObserverT &observer) { if (curr != next) { diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h index e44b1c4e1..d824fdb83 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl_replay.h @@ -556,22 +556,20 @@ public: bool record_incremental(ModuleT &module) { assert(streaming); - struct : public observer { + struct { std::unordered_map *ident_lookup; spool::writer *writer; CXXRTL_ALWAYS_INLINE - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) override { + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) { writer->write_change(ident_lookup->at(base), chunks, value); } CXXRTL_ALWAYS_INLINE - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) override { + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) { writer->write_change(ident_lookup->at(base), chunks, value, index); } - } record_observer; - record_observer.ident_lookup = &ident_lookup; - record_observer.writer = &writer; + } record_observer = { &ident_lookup, &writer }; writer.write_sample(/*incremental=*/true, pointer++, timestamp); for (auto input_index : inputs) { From a33acb7cd9a2366df55036db039b01b72efcc60b Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 09:08:07 +0000 Subject: [PATCH 3/6] cxxrtl: refactor the formatter and use a closure. This commit achieves three roughly equally important goals: 1. To bring the rendering code in kernel/fmt.cc and in cxxrtl.h as close together as possible, with an ideal of only having the bigint library as the difference between the render functions. 2. To make the treatment of `$time` and `$realtime` in CXXRTL closer to the Verilog semantics, at least in the formatting code. 3. To change the code generator so that all of the `$print`-to-`string` conversion code is contained inside of a closure. There are two reasons to aim for goal (3): a. Because output redirection through definition of a global ostream object is neither convenient nor useful for environments where the output is consumed by other code rather than being printed on a terminal. b. Because it may be desirable to, in some cases, ignore the `$print` cells that are present in the netlist based on a runtime decision. This is doubly true for an upcoming `$check` cell implementing assertions, since failing a `$check` would by default cause a crash. --- backends/cxxrtl/cxxrtl_backend.cc | 9 +- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 220 ++++++++++++++---------- kernel/fmt.cc | 124 ++++++------- kernel/fmt.h | 5 +- tests/fmt/always_full_tb.cc | 7 +- 5 files changed, 192 insertions(+), 173 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index 30995ea11..e4a000627 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -1072,9 +1072,12 @@ struct CxxrtlWorker { dump_sigspec_rhs(cell->getPort(ID::EN)); f << " == value<1>{1u}) {\n"; inc_indent(); - f << indent << print_output; - fmt.emit_cxxrtl(f, [this](const RTLIL::SigSpec &sig) { dump_sigspec_rhs(sig); }); - f << ";\n"; + f << indent << "auto formatter = [&](int64_t itime, double ftime) {\n"; + inc_indent(); + fmt.emit_cxxrtl(f, indent, [this](const RTLIL::SigSpec &sig) { dump_sigspec_rhs(sig); }); + dec_indent(); + f << indent << "};\n"; + f << indent << print_output << " << formatter(steps, steps);\n"; dec_indent(); f << indent << "}\n"; } diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index bd336f481..654f13b4a 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -565,7 +565,7 @@ struct value : public expr_base> { } value neg() const { - return value { 0u }.sub(*this); + return value().sub(*this); } bool ucmp(const value &other) const { @@ -763,102 +763,134 @@ std::ostream &operator<<(std::ostream &os, const value &val) { return os; } -template -struct value_formatted { - const value &val; - bool character; - bool justify_left; - char padding; - int width; - int base; - bool signed_; - bool plus; +// Must be kept in sync with `struct FmtPart` in kernel/fmt.h! +// Default member initializers would make this a non-aggregate-type in C++11, so they are commented out. +struct fmt_part { + enum { + STRING = 0, + INTEGER = 1, + CHARACTER = 2, + TIME = 3, + } type; - value_formatted(const value &val, bool character, bool justify_left, char padding, int width, int base, bool signed_, bool plus) : - val(val), character(character), justify_left(justify_left), padding(padding), width(width), base(base), signed_(signed_), plus(plus) {} - value_formatted(const value_formatted &) = delete; - value_formatted &operator=(const value_formatted &rhs) = delete; + // STRING type + std::string str; + + // INTEGER/CHARACTER types + // + value val; + + // INTEGER/CHARACTER/TIME types + enum { + RIGHT = 0, + LEFT = 1, + } justify; // = RIGHT; + char padding; // = '\0'; + size_t width; // = 0; + + // INTEGER type + unsigned base; // = 10; + bool signed_; // = false; + bool plus; // = false; + + // TIME type + bool realtime; // = false; + // + int64_t itime; + // + double ftime; + + // Format the part as a string. + // + // The values of `itime` and `ftime` are used for `$time` and `$realtime`, correspondingly. + template + std::string render(value val, int64_t itime, double ftime) + { + // We might want to replace some of these bit() calls with direct + // chunk access if it turns out to be slow enough to matter. + std::string buf; + switch (type) { + case STRING: + return str; + + case CHARACTER: { + buf.reserve(Bits/8); + for (int i = 0; i < Bits; i += 8) { + char ch = 0; + for (int j = 0; j < 8 && i + j < int(Bits); j++) + if (val.bit(i + j)) + ch |= 1 << j; + if (ch != 0) + buf.append({ch}); + } + std::reverse(buf.begin(), buf.end()); + break; + } + + case INTEGER: { + size_t width = Bits; + if (base != 10) { + width = 0; + for (size_t index = 0; index < Bits; index++) + if (val.bit(index)) + width = index + 1; + } + + if (base == 2) { + for (size_t i = width; i > 0; i--) + buf += (val.bit(i - 1) ? '1' : '0'); + } else if (base == 8 || base == 16) { + size_t step = (base == 16) ? 4 : 3; + for (size_t index = 0; index < width; index += step) { + uint8_t value = val.bit(index) | (val.bit(index + 1) << 1) | (val.bit(index + 2) << 2); + if (step == 4) + value |= val.bit(index + 3) << 3; + buf += "0123456789abcdef"[value]; + } + std::reverse(buf.begin(), buf.end()); + } else if (base == 10) { + bool negative = signed_ && val.is_neg(); + if (negative) + val = val.neg(); + if (val.is_zero()) + buf += '0'; + value<(Bits > 4 ? Bits : 4)> xval = val.template zext<(Bits > 4 ? Bits : 4)>(); + while (!xval.is_zero()) { + value<(Bits > 4 ? Bits : 4)> quotient, remainder; + if (Bits >= 4) + std::tie(quotient, remainder) = xval.udivmod(value<(Bits > 4 ? Bits : 4)>{10u}); + else + std::tie(quotient, remainder) = std::make_pair(value<(Bits > 4 ? Bits : 4)>{0u}, xval); + buf += '0' + remainder.template trunc<4>().template get(); + xval = quotient; + } + if (negative || plus) + buf += negative ? '-' : '+'; + std::reverse(buf.begin(), buf.end()); + } else assert(false && "Unsupported base for fmt_part"); + break; + } + + case TIME: { + buf = realtime ? std::to_string(ftime) : std::to_string(itime); + break; + } + } + + std::string str; + assert(width == 0 || padding != '\0'); + if (justify == RIGHT && buf.size() < width) { + size_t pad_width = width - buf.size(); + if (padding == '0' && (buf.front() == '+' || buf.front() == '-')) { + str += buf.front(); + buf.erase(0, 1); + } + str += std::string(pad_width, padding); + } + str += buf; + if (justify == LEFT && buf.size() < width) + str += std::string(width - buf.size(), padding); + return str; + } }; -template -std::ostream &operator<<(std::ostream &os, const value_formatted &vf) -{ - value val = vf.val; - - std::string buf; - - // We might want to replace some of these bit() calls with direct - // chunk access if it turns out to be slow enough to matter. - - if (!vf.character) { - size_t width = Bits; - if (vf.base != 10) { - width = 0; - for (size_t index = 0; index < Bits; index++) - if (val.bit(index)) - width = index + 1; - } - - if (vf.base == 2) { - for (size_t i = width; i > 0; i--) - buf += (val.bit(i - 1) ? '1' : '0'); - } else if (vf.base == 8 || vf.base == 16) { - size_t step = (vf.base == 16) ? 4 : 3; - for (size_t index = 0; index < width; index += step) { - uint8_t value = val.bit(index) | (val.bit(index + 1) << 1) | (val.bit(index + 2) << 2); - if (step == 4) - value |= val.bit(index + 3) << 3; - buf += "0123456789abcdef"[value]; - } - std::reverse(buf.begin(), buf.end()); - } else if (vf.base == 10) { - bool negative = vf.signed_ && val.is_neg(); - if (negative) - val = val.neg(); - if (val.is_zero()) - buf += '0'; - while (!val.is_zero()) { - value quotient, remainder; - if (Bits >= 4) - std::tie(quotient, remainder) = val.udivmod(value{10u}); - else - std::tie(quotient, remainder) = std::make_pair(value{0u}, val); - buf += '0' + remainder.template trunc<(Bits > 4 ? 4 : Bits)>().val().template get(); - val = quotient; - } - if (negative || vf.plus) - buf += negative ? '-' : '+'; - std::reverse(buf.begin(), buf.end()); - } else assert(false); - } else { - buf.reserve(Bits/8); - for (int i = 0; i < Bits; i += 8) { - char ch = 0; - for (int j = 0; j < 8 && i + j < int(Bits); j++) - if (val.bit(i + j)) - ch |= 1 << j; - if (ch != 0) - buf.append({ch}); - } - std::reverse(buf.begin(), buf.end()); - } - - assert(vf.width == 0 || vf.padding != '\0'); - if (!vf.justify_left && buf.size() < vf.width) { - size_t pad_width = vf.width - buf.size(); - if (vf.padding == '0' && (buf.front() == '+' || buf.front() == '-')) { - os << buf.front(); - buf.erase(0, 1); - } - os << std::string(pad_width, vf.padding); - } - os << buf; - if (vf.justify_left && buf.size() < vf.width) - os << std::string(vf.width - buf.size(), vf.padding); - - return os; -} - // An object that can be passed to a `commit()` method in order to produce a replay log of every state change in // the simulation. struct observer { diff --git a/kernel/fmt.cc b/kernel/fmt.cc index 383fa7de1..7d16b20c1 100644 --- a/kernel/fmt.cc +++ b/kernel/fmt.cc @@ -569,82 +569,60 @@ std::vector Fmt::emit_verilog() const return args; } -void Fmt::emit_cxxrtl(std::ostream &f, std::function emit_sig) const +std::string escape_cxx_string(const std::string &input) { - for (auto &part : parts) { - switch (part.type) { - case FmtPart::STRING: - f << " << \""; - for (char c : part.str) { - switch (c) { - case '\\': - YS_FALLTHROUGH - case '"': - f << '\\' << c; - break; - case '\a': - f << "\\a"; - break; - case '\b': - f << "\\b"; - break; - case '\f': - f << "\\f"; - break; - case '\n': - f << "\\n"; - break; - case '\r': - f << "\\r"; - break; - case '\t': - f << "\\t"; - break; - case '\v': - f << "\\v"; - break; - default: - f << c; - break; - } - } - f << '"'; - break; - - case FmtPart::INTEGER: - case FmtPart::CHARACTER: { - f << " << value_formatted<" << part.sig.size() << ">("; - emit_sig(part.sig); - f << ", " << (part.type == FmtPart::CHARACTER); - f << ", " << (part.justify == FmtPart::LEFT); - f << ", (char)" << (int)part.padding; - f << ", " << part.width; - f << ", " << part.base; - f << ", " << part.signed_; - f << ", " << part.plus; - f << ')'; - break; - } - - case FmtPart::TIME: { - // CXXRTL only records steps taken, so there's no difference between - // the values taken by $time and $realtime. - f << " << value_formatted<64>("; - f << "value<64>{steps}"; - f << ", " << (part.type == FmtPart::CHARACTER); - f << ", " << (part.justify == FmtPart::LEFT); - f << ", (char)" << (int)part.padding; - f << ", " << part.width; - f << ", " << part.base; - f << ", " << part.signed_; - f << ", " << part.plus; - f << ')'; - break; - } - - default: log_abort(); + std::string output = "\""; + for (auto c : input) { + if (::isprint(c)) { + if (c == '\\') + output.push_back('\\'); + output.push_back(c); + } else { + char l = c & 0xf, h = (c >> 4) & 0xf; + output.append("\\x"); + output.push_back((h < 10 ? '0' + h : 'a' + h - 10)); + output.push_back((l < 10 ? '0' + l : 'a' + l - 10)); } } + output.push_back('"'); + if (output.find('\0') != std::string::npos) { + output.insert(0, "std::string {"); + output.append(stringf(", %zu}", input.size())); + } + return output; +} + +void Fmt::emit_cxxrtl(std::ostream &os, std::string indent, std::function emit_sig) const +{ + os << indent << "std::string buf;\n"; + for (auto &part : parts) { + os << indent << "buf += fmt_part { "; + os << "fmt_part::"; + switch (part.type) { + case FmtPart::STRING: os << "STRING"; break; + case FmtPart::INTEGER: os << "INTEGER"; break; + case FmtPart::CHARACTER: os << "CHARACTER"; break; + case FmtPart::TIME: os << "TIME"; break; + } + os << ", "; + os << escape_cxx_string(part.str) << ", "; + os << "fmt_part::"; + switch (part.justify) { + case FmtPart::LEFT: os << "LEFT"; break; + case FmtPart::RIGHT: os << "RIGHT"; break; + } + os << ", "; + os << "(char)" << (int)part.padding << ", "; + os << part.width << ", "; + os << part.base << ", "; + os << part.signed_ << ", "; + os << part.plus << ", "; + os << part.realtime; + os << " }.render("; + emit_sig(part.sig); + os << ", itime, ftime);\n"; + } + os << indent << "return buf;\n"; } std::string Fmt::render() const diff --git a/kernel/fmt.h b/kernel/fmt.h index 39a278c56..126d020c0 100644 --- a/kernel/fmt.h +++ b/kernel/fmt.h @@ -50,6 +50,7 @@ struct VerilogFmtArg { // RTLIL format part, such as the substitutions in: // "foo {4:> 4du} bar {2:<01hs}" +// Must be kept in sync with `struct fmt_part` in backends/cxxrtl/runtime/cxxrtl/cxxrtl.h! struct FmtPart { enum { STRING = 0, @@ -71,7 +72,7 @@ struct FmtPart { } justify = RIGHT; char padding = '\0'; size_t width = 0; - + // INTEGER type unsigned base = 10; bool signed_ = false; @@ -93,7 +94,7 @@ public: void parse_verilog(const std::vector &args, bool sformat_like, int default_base, RTLIL::IdString task_name, RTLIL::IdString module_name); std::vector emit_verilog() const; - void emit_cxxrtl(std::ostream &f, std::function emit_sig) const; + void emit_cxxrtl(std::ostream &os, std::string indent, std::function emit_sig) const; std::string render() const; diff --git a/tests/fmt/always_full_tb.cc b/tests/fmt/always_full_tb.cc index 229f78aeb..a29a8ae05 100644 --- a/tests/fmt/always_full_tb.cc +++ b/tests/fmt/always_full_tb.cc @@ -2,8 +2,13 @@ int main() { + struct : public performer { + int64_t time() const override { return 1; } + void on_print(const std::string &output, const cxxrtl::metadata_map &) override { std::cerr << output; } + } performer; + cxxrtl_design::p_always__full uut; uut.p_clk.set(!uut.p_clk); - uut.step(); + uut.step(&performer); return 0; } From 02e3d508fad9c07692022555c8cefd76575d8c98 Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 10:45:22 +0000 Subject: [PATCH 4/6] cxxrtl: remove `module::steps`. (breaking change) This approach to tracking simulation time was a mistake that I did not catch in review. It has several issues: 1. There is absolutely no requirement to call `step()`, as it is a convenience function. In particular, `steps` will not be incremented in submodules if `-noflatten` is used. 2. The semantics of `steps` does not match that of the Verilog `$time` construct. 3. There is no way to make the semantics of `%t` match that of Verilog. 4. The `module` interface is intentionally very barebones. It is little more than a container for three method pointers, `reset`, `eval`, and `commit`. Adding ancillary data to it goes against this. If similar functionality is introduced again it should probably be a variable that is global per toplevel design using some object that is unique for an entire hierarchy of modules, and ideally exposed via the C API. For now, it is being removed (in this commit) and (in next commit) the capability is being reintroduced through a context object that can be specified for `eval()`. --- backends/cxxrtl/cxxrtl_backend.cc | 2 +- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index e4a000627..3b0c2827d 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -1077,7 +1077,7 @@ struct CxxrtlWorker { fmt.emit_cxxrtl(f, indent, [this](const RTLIL::SigSpec &sig) { dump_sigspec_rhs(sig); }); dec_indent(); f << indent << "};\n"; - f << indent << print_output << " << formatter(steps, steps);\n"; + f << indent << print_output << " << formatter(0, 0.0);\n"; dec_indent(); f << indent << "}\n"; } diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index 654f13b4a..7d6a75d4a 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -1331,10 +1331,7 @@ struct module { virtual bool eval() = 0; virtual bool commit() = 0; - unsigned int steps = 0; - size_t step() { - ++steps; size_t deltas = 0; bool converged = false; do { From 905f07c03fedefd2be6333ab5425b2623955630f Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 11:12:32 +0000 Subject: [PATCH 5/6] cxxrtl: introduce `performer`, a context object for `eval()`. (breaking change) At the moment the only thing it allows is redirecting `$print` cell output in a context-dependent manner. In the future, it will allow customizing handling of `$check` cells (where the default is to abort), of out-of-range memory accesses, and other runtime conditions with effects. This context object also allows a suitably written testbench to add Verilog-compliant `$time`/`$realtime` handling, albeit it involves the ceremony of defining a `performer` subclass. Most people will want something like this to customize `$time`: int64_t time = 0; struct : public performer { int64_t *time_ptr; int64_t time() const override { return *time_ptr; } } performer = { &time }; p_top.step(&performer); --- backends/cxxrtl/cxxrtl_backend.cc | 34 ++++++++++++++----------- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 24 +++++++++++++---- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index 3b0c2827d..c6a4265fc 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -1077,7 +1077,15 @@ struct CxxrtlWorker { fmt.emit_cxxrtl(f, indent, [this](const RTLIL::SigSpec &sig) { dump_sigspec_rhs(sig); }); dec_indent(); f << indent << "};\n"; - f << indent << print_output << " << formatter(0, 0.0);\n"; + f << indent << "if (performer) {\n"; + inc_indent(); + f << indent << "performer->on_print(formatter(performer->time(), performer->realtime()));\n"; + dec_indent(); + f << indent << "} else {\n"; + inc_indent(); + f << indent << print_output << " << formatter(0, 0.0);\n"; + dec_indent(); + f << indent << "}\n"; dec_indent(); f << indent << "}\n"; } @@ -1497,11 +1505,11 @@ struct CxxrtlWorker { }; if (buffered_inputs) { // If we have any buffered inputs, there's no chance of converging immediately. - f << indent << mangle(cell) << access << "eval();\n"; + f << indent << mangle(cell) << access << "eval(performer);\n"; f << indent << "converged = false;\n"; assign_from_outputs(/*cell_converged=*/false); } else { - f << indent << "if (" << mangle(cell) << access << "eval()) {\n"; + f << indent << "if (" << mangle(cell) << access << "eval(performer)) {\n"; inc_indent(); assign_from_outputs(/*cell_converged=*/true); dec_indent(); @@ -2384,7 +2392,8 @@ struct CxxrtlWorker { dump_reset_method(module); f << indent << "}\n"; f << "\n"; - f << indent << "bool eval() override {\n"; + // No default argument, to prevent unintentional `return bb_foo::eval();` calls that drop performer. + f << indent << "bool eval(performer *performer) override {\n"; dump_eval_method(module); f << indent << "}\n"; f << "\n"; @@ -2481,7 +2490,7 @@ struct CxxrtlWorker { f << "\n"; f << indent << "void reset() override;\n"; f << "\n"; - f << indent << "bool eval() override;\n"; + f << indent << "bool eval(performer *performer = nullptr) override;\n"; f << "\n"; f << indent << "template\n"; f << indent << "bool commit(ObserverT &observer) {\n"; @@ -2520,7 +2529,7 @@ struct CxxrtlWorker { dump_reset_method(module); f << indent << "}\n"; f << "\n"; - f << indent << "bool " << mangle(module) << "::eval() {\n"; + f << indent << "bool " << mangle(module) << "::eval(performer *performer) {\n"; dump_eval_method(module); f << indent << "}\n"; if (debug_info) { @@ -2544,7 +2553,6 @@ struct CxxrtlWorker { RTLIL::Module *top_module = nullptr; std::vector modules; TopoSort topo_design; - bool has_prints = false; for (auto module : design->modules()) { if (!design->selected_module(module)) continue; @@ -2557,8 +2565,6 @@ struct CxxrtlWorker { topo_design.node(module); for (auto cell : module->cells()) { - if (cell->type == ID($print)) - has_prints = true; if (is_internal_cell(cell->type) || is_cxxrtl_blackbox_cell(cell)) continue; RTLIL::Module *cell_module = design->module(cell->type); @@ -2617,8 +2623,6 @@ struct CxxrtlWorker { f << "#include \"" << basename(intf_filename) << "\"\n"; else f << "#include \n"; - if (has_prints) - f << "#include \n"; f << "\n"; f << "#if defined(CXXRTL_INCLUDE_CAPI_IMPL) || \\\n"; f << " defined(CXXRTL_INCLUDE_VCD_CAPI_IMPL)\n"; @@ -3296,7 +3300,7 @@ struct CxxrtlBackend : public Backend { log(" value<8> p_i_data;\n"); log(" wire<8> p_o_data;\n"); log("\n"); - log(" bool eval() override;\n"); + log(" bool eval(performer *performer) override;\n"); log(" template\n"); log(" bool commit(ObserverT &observer);\n"); log(" bool commit() override;\n"); @@ -3311,11 +3315,11 @@ struct CxxrtlBackend : public Backend { log(" namespace cxxrtl_design {\n"); log("\n"); log(" struct stderr_debug : public bb_p_debug {\n"); - log(" bool eval() override {\n"); + log(" bool eval(performer *performer) override {\n"); log(" if (posedge_p_clk() && p_en)\n"); log(" fprintf(stderr, \"debug: %%02x\\n\", p_i_data.data[0]);\n"); log(" p_o_data.next = p_i_data;\n"); - log(" return bb_p_debug::eval();\n"); + log(" return bb_p_debug::eval(performer);\n"); log(" }\n"); log(" };\n"); log("\n"); @@ -3416,7 +3420,7 @@ struct CxxrtlBackend : public Backend { log(" -print-output \n"); log(" $print cells in the generated code direct their output to .\n"); log(" must be one of \"std::cout\", \"std::cerr\". if not specified,\n"); - log(" \"std::cout\" is used.\n"); + log(" \"std::cout\" is used. explicitly provided performer overrides this.\n"); log("\n"); log(" -nohierarchy\n"); log(" use design hierarchy as-is. in most designs, a top module should be\n"); diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index 7d6a75d4a..d15772a3c 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -39,6 +39,7 @@ #include #include #include +#include // `cxxrtl::debug_item` has to inherit from `cxxrtl_object` to satisfy strict aliasing requirements. #include @@ -891,8 +892,21 @@ struct fmt_part { } }; +// An object that can be passed to a `eval()` method in order to act on side effects. +struct performer { + // Called to evaluate a Verilog `$time` expression. + virtual int64_t time() const { return 0; } + + // Called to evaluate a Verilog `$realtime` expression. + virtual double realtime() const { return time(); } + + // Called when a `$print` cell is triggered. + virtual void on_print(const std::string &output) { std::cout << output; } +}; + // An object that can be passed to a `commit()` method in order to produce a replay log of every state change in -// the simulation. +// the simulation. Unlike `performer`, `observer` does not use virtual calls as their overhead is unacceptable, and +// a comparatively heavyweight template-based solution is justified. struct observer { // Called when the `commit()` method for a wire is about to update the `chunks` chunks at `base` with `chunks` chunks // at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to the wire chunk count and @@ -1328,14 +1342,14 @@ struct module { virtual void reset() = 0; - virtual bool eval() = 0; - virtual bool commit() = 0; + virtual bool eval(performer *performer = nullptr) = 0; + virtual bool commit() = 0; // commit observer isn't available since it avoids virtual calls - size_t step() { + size_t step(performer *performer = nullptr) { size_t deltas = 0; bool converged = false; do { - converged = eval(); + converged = eval(performer); deltas++; } while (commit() && !converged); return deltas; From 5a1fcdea137585ecaffb602dcd1fa324984cce0a Mon Sep 17 00:00:00 2001 From: Catherine Date: Tue, 16 Jan 2024 16:17:10 +0000 Subject: [PATCH 6/6] cxxrtl: include attributes in `performer::on_print` callback. This is useful primarily to determine the source location, but can also be used for any other purpose. --- backends/cxxrtl/cxxrtl_backend.cc | 5 ++- backends/cxxrtl/runtime/cxxrtl/cxxrtl.h | 54 ++++++++++++------------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index c6a4265fc..ef718067f 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -1079,7 +1079,10 @@ struct CxxrtlWorker { f << indent << "};\n"; f << indent << "if (performer) {\n"; inc_indent(); - f << indent << "performer->on_print(formatter(performer->time(), performer->realtime()));\n"; + f << indent << "static const metadata_map attributes = "; + dump_metadata_map(cell->attributes); + f << ";\n"; + f << indent << "performer->on_print(formatter(performer->time(), performer->realtime()), attributes);\n"; dec_indent(); f << indent << "} else {\n"; inc_indent(); diff --git a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h index d15772a3c..d3b9cf9db 100644 --- a/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/runtime/cxxrtl/cxxrtl.h @@ -892,33 +892,6 @@ struct fmt_part { } }; -// An object that can be passed to a `eval()` method in order to act on side effects. -struct performer { - // Called to evaluate a Verilog `$time` expression. - virtual int64_t time() const { return 0; } - - // Called to evaluate a Verilog `$realtime` expression. - virtual double realtime() const { return time(); } - - // Called when a `$print` cell is triggered. - virtual void on_print(const std::string &output) { std::cout << output; } -}; - -// An object that can be passed to a `commit()` method in order to produce a replay log of every state change in -// the simulation. Unlike `performer`, `observer` does not use virtual calls as their overhead is unacceptable, and -// a comparatively heavyweight template-based solution is justified. -struct observer { - // Called when the `commit()` method for a wire is about to update the `chunks` chunks at `base` with `chunks` chunks - // at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to the wire chunk count and - // `base` points to the first chunk. - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) {} - - // Called when the `commit()` method for a memory is about to update the `chunks` chunks at `&base[chunks * index]` - // with `chunks` chunks at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to - // the memory element chunk count and `base` points to the first chunk of the first element of the memory. - void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) {} -}; - template struct wire { static constexpr size_t bits = Bits; @@ -1100,6 +1073,33 @@ struct metadata { typedef std::map metadata_map; +// An object that can be passed to a `eval()` method in order to act on side effects. +struct performer { + // Called to evaluate a Verilog `$time` expression. + virtual int64_t time() const { return 0; } + + // Called to evaluate a Verilog `$realtime` expression. + virtual double realtime() const { return time(); } + + // Called when a `$print` cell is triggered. + virtual void on_print(const std::string &output, const metadata_map &attributes) { std::cout << output; } +}; + +// An object that can be passed to a `commit()` method in order to produce a replay log of every state change in +// the simulation. Unlike `performer`, `observer` does not use virtual calls as their overhead is unacceptable, and +// a comparatively heavyweight template-based solution is justified. +struct observer { + // Called when the `commit()` method for a wire is about to update the `chunks` chunks at `base` with `chunks` chunks + // at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to the wire chunk count and + // `base` points to the first chunk. + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value) {} + + // Called when the `commit()` method for a memory is about to update the `chunks` chunks at `&base[chunks * index]` + // with `chunks` chunks at `value` that have a different bit pattern. It is guaranteed that `chunks` is equal to + // the memory element chunk count and `base` points to the first chunk of the first element of the memory. + void on_update(size_t chunks, const chunk_t *base, const chunk_t *value, size_t index) {} +}; + // Tag class to disambiguate values/wires and their aliases. struct debug_alias {};