From fb3704c8964aac37b6745647cb6338d464ff38fb Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 03:21:08 +0000 Subject: cxxrtl: minor debug-related improvements. --- backends/cxxrtl/cxxrtl_backend.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index 64af5dab8..e2fd6b672 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -2419,7 +2419,8 @@ struct CxxrtlBackend : public Backend { log(" no debug information.\n"); log("\n"); log(" -g1\n"); - log(" debug information for non-localized public wires.\n"); + log(" debug information for non-optimized public wires. this also makes it\n"); + log(" possible to use the C API.\n"); log("\n"); } @@ -2494,7 +2495,7 @@ struct CxxrtlBackend : public Backend { case 0: break; default: - log_cmd_error("Invalid optimization level %d.\n", opt_level); + log_cmd_error("Invalid debug information level %d.\n", debug_level); } std::ofstream intf_f; -- cgit v1.2.3 From 8262997c4ea1cb6acafb5d8f29f1300da2746d5e Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 04:08:09 +0000 Subject: cxxrtl: fix typo in comment. NFC. --- backends/cxxrtl/cxxrtl_backend.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index e2fd6b672..a43a23d04 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -359,10 +359,10 @@ struct FlowGraph { // // eliminating the unnecessary delta cycle. Conceptually, the CELL_SYNC node type is a series of // connections of the form `connect \lhs \cell.\sync_output`; the right-hand side of these is not - // as a wire in RTLIL. If it was expressible, then `\cell.\sync_output` would have a sync def, - // and this node would be an ordinary CONNECT node, with `\lhs` having a comb def. Because it isn't, - // a special node type is used, the right-hand side does not appear anywhere, and the left-hand - // side has a comb def. + // expressible as a wire in RTLIL. If it was expressible, then `\cell.\sync_output` would have + // a sync def, and this node would be an ordinary CONNECT node, with `\lhs` having a comb def. + // Because it isn't, a special node type is used, the right-hand side does not appear anywhere, + // and the left-hand side has a comb def. for (auto conn : cell->connections()) if (cell->output(conn.first)) if (is_cxxrtl_sync_port(cell, conn.first)) { -- cgit v1.2.3 From a0466e1a969a40a7e90f8bce244eba6cdc1f2c1c Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 12:55:11 +0000 Subject: cxxrtl: add missing installs of include files. --- Makefile | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Makefile b/Makefile index e589b2b67..51733d404 100644 --- a/Makefile +++ b/Makefile @@ -588,6 +588,11 @@ $(eval $(call add_include_file,passes/fsm/fsmdata.h)) $(eval $(call add_include_file,frontends/ast/ast.h)) $(eval $(call add_include_file,backends/ilang/ilang_backend.h)) $(eval $(call add_include_file,backends/cxxrtl/cxxrtl.h)) +$(eval $(call add_include_file,backends/cxxrtl/cxxrtl_vcd.h)) +$(eval $(call add_include_file,backends/cxxrtl/cxxrtl_capi.cc)) +$(eval $(call add_include_file,backends/cxxrtl/cxxrtl_capi.h)) +$(eval $(call add_include_file,backends/cxxrtl/cxxrtl_vcd_capi.cc)) +$(eval $(call add_include_file,backends/cxxrtl/cxxrtl_vcd_capi.h)) OBJS += kernel/driver.o kernel/register.o kernel/rtlil.o kernel/log.o kernel/calc.o kernel/yosys.o OBJS += kernel/cellaigs.o kernel/celledges.o -- cgit v1.2.3 From 9b39c6f7442a525c8dbfd94f1af65a0b606e648b Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 16:22:30 +0000 Subject: cxxrtl: emit debug information for alias wires. Alias wires can represent a significant chunk of the design in highly hierarchical designs; in Minerva SRAM, there are 273 member wires and 527 alias wires. Showing them in every hierarchy level significantly improves usability. --- backends/cxxrtl/cxxrtl_backend.cc | 58 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index a43a23d04..01caa3793 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -539,6 +539,7 @@ struct CxxrtlWorker { dict elided_wires; dict> schedule; pool localized_wires; + dict debug_alias_wires; dict> blackbox_specializations; dict eval_converges; @@ -1606,15 +1607,29 @@ struct CxxrtlWorker { void dump_debug_info_method(RTLIL::Module *module) { + size_t count_member_wires = 0; + size_t count_alias_wires = 0; + size_t count_skipped_wires = 0; inc_indent(); f << indent << "assert(path.empty() || path[path.size() - 1] == ' ');\n"; for (auto wire : module->wires()) { if (wire->name[0] != '\\') continue; - if (localized_wires.count(wire)) + if (debug_alias_wires.count(wire)) { + // Alias of a member wire + f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); + f << ", debug_item(" << mangle(debug_alias_wires[wire]) << "));\n"; + count_alias_wires++; continue; - f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); - f << ", debug_item(" << mangle(wire) << "));\n"; + } + if (!localized_wires.count(wire)) { + // Member wire + f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); + f << ", debug_item(" << mangle(wire) << "));\n"; + count_member_wires++; + continue; + } + count_skipped_wires++; } for (auto &memory_it : module->memories) { if (memory_it.first[0] != '\\') @@ -1630,6 +1645,11 @@ struct CxxrtlWorker { f << "path + " << escape_cxx_string(get_hdl_name(cell) + ' ') << ");\n"; } dec_indent(); + + log_debug("Debug information statistics for module %s:\n", log_id(module)); + log_debug(" Member wires: %zu\n", count_member_wires); + log_debug(" Alias wires: %zu\n", count_alias_wires); + log_debug(" Other wires: %zu (no debug information)\n", count_skipped_wires); } void dump_metadata_map(const dict &metadata_map) @@ -2141,6 +2161,38 @@ struct CxxrtlWorker { } eval_converges[module] = feedback_wires.empty() && buffered_wires.empty(); + + if (debug_info) { + // Find wires that alias other wires; debug information can be enriched with these at essentially zero + // additional cost. + // + // Note that the information collected here can't be used for optimizing the netlist: debug information queries + // are pure and run on a design in a stable state, which allows assumptions that do not otherwise hold. + for (auto wire : module->wires()) { + if (wire->name[0] != '\\') + continue; + if (!localized_wires[wire]) + continue; + const RTLIL::Wire *wire_it = wire; + while (1) { + if (!(flow.wire_def_elidable.count(wire_it) && flow.wire_def_elidable[wire_it])) + break; // not an alias: complex def + log_assert(flow.wire_comb_defs[wire_it].size() == 1); + FlowGraph::Node *node = *flow.wire_comb_defs[wire_it].begin(); + if (node->connect.second.is_wire()) { + RTLIL::Wire *rhs_wire = node->connect.second.as_wire(); + if (localized_wires[rhs_wire]) { + wire_it = rhs_wire; // maybe an alias + } else { + debug_alias_wires[wire] = rhs_wire; // is an alias + break; + } + } else { + break; // not an alias: complex rhs + } + } + } + } } if (has_feedback_arcs || has_buffered_wires) { // Although both non-feedback buffered combinatorial wires and apparent feedback wires may be eliminated -- cgit v1.2.3 From d5c07e5b6f5c91666867751233f5c537068a7136 Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 16:36:26 +0000 Subject: cxxrtl: track aliases in VCD writer. This commit changes the VCD writer such that for all signals that share `debug_item.curr`, it would only emit a single VCD identifier, and sample the value once. Commit 9b39c6f7 added redundancy to debug information by including alias wires, and increased the size of VCD files proportionally; this commit eliminates the redundancy from VCD files so that their size is the same as before. --- backends/cxxrtl/cxxrtl_vcd.h | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_vcd.h b/backends/cxxrtl/cxxrtl_vcd.h index 5706917ca..8ba94ea77 100644 --- a/backends/cxxrtl/cxxrtl_vcd.h +++ b/backends/cxxrtl/cxxrtl_vcd.h @@ -34,6 +34,7 @@ class vcd_writer { std::vector current_scope; std::vector variables; std::vector cache; + std::map aliases; bool streaming = false; void emit_timescale(unsigned number, const std::string &unit) { @@ -103,10 +104,16 @@ class vcd_writer { buffer += '\n'; } - void append_variable(size_t width, chunk_t *curr) { - const size_t chunks = (width + (sizeof(chunk_t) * 8 - 1)) / (sizeof(chunk_t) * 8); - variables.emplace_back(variable { variables.size(), width, curr, cache.size() }); - cache.insert(cache.end(), &curr[0], &curr[chunks]); + const variable ®ister_variable(size_t width, chunk_t *curr) { + if (aliases.count(curr)) { + return variables[aliases[curr]]; + } else { + const size_t chunks = (width + (sizeof(chunk_t) * 8 - 1)) / (sizeof(chunk_t) * 8); + aliases[curr] = variables.size(); + variables.emplace_back(variable { variables.size(), width, curr, cache.size() }); + cache.insert(cache.end(), &curr[0], &curr[chunks]); + return variables.back(); + } } bool test_variable(const variable &var) { @@ -151,20 +158,17 @@ public: switch (item.type) { // Not the best naming but oh well... case debug_item::VALUE: - append_variable(item.width, item.curr); - emit_var(variables.back(), "wire", name); + emit_var(register_variable(item.width, item.curr), "wire", name); break; case debug_item::WIRE: - append_variable(item.width, item.curr); - emit_var(variables.back(), "reg", name); + emit_var(register_variable(item.width, item.curr), "reg", name); break; case debug_item::MEMORY: { const size_t stride = (item.width + (sizeof(chunk_t) * 8 - 1)) / (sizeof(chunk_t) * 8); for (size_t index = 0; index < item.depth; index++) { chunk_t *nth_curr = &item.curr[stride * index]; std::string nth_name = name + '[' + std::to_string(index) + ']'; - append_variable(item.width, nth_curr); - emit_var(variables.back(), "reg", nth_name); + emit_var(register_variable(item.width, nth_curr), "reg", nth_name); } break; } -- cgit v1.2.3 From f2d7a18756088f4167b91c5ec4735bb694f36567 Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 17:29:08 +0000 Subject: cxxrtl: emit debug information for constant wires. Constant wires can represent a significant chunk of the design in generic designs or after optimization. Emitting them in VCD files significantly improves usability because gtkwave removes all traces that are not present in the VCD file after reload, and iterative development suffers if switching a varying signal to a constant disrupts the workflow. --- backends/cxxrtl/cxxrtl.h | 11 +++++++++++ backends/cxxrtl/cxxrtl_backend.cc | 39 +++++++++++++++++++++++++++------------ backends/cxxrtl/cxxrtl_capi.h | 11 ++++++----- 3 files changed, 44 insertions(+), 17 deletions(-) diff --git a/backends/cxxrtl/cxxrtl.h b/backends/cxxrtl/cxxrtl.h index 5f74899fd..30f4667c5 100644 --- a/backends/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/cxxrtl.h @@ -741,6 +741,17 @@ struct debug_item : ::cxxrtl_object { next = item.data; } + template + debug_item(const value &item) { + static_assert(sizeof(item) == value::chunks * sizeof(chunk_t), + "value is not compatible with C layout"); + type = VALUE; + width = Bits; + depth = 1; + curr = const_cast(item.data); + next = nullptr; + } + template debug_item(wire &item) { static_assert(sizeof(item.curr) == value::chunks * sizeof(chunk_t) && diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index 01caa3793..b3aec2110 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -540,6 +540,7 @@ struct CxxrtlWorker { dict> schedule; pool localized_wires; dict debug_alias_wires; + dict debug_const_wires; dict> blackbox_specializations; dict eval_converges; @@ -1607,29 +1608,36 @@ struct CxxrtlWorker { void dump_debug_info_method(RTLIL::Module *module) { - size_t count_member_wires = 0; + size_t count_const_wires = 0; size_t count_alias_wires = 0; + size_t count_member_wires = 0; size_t count_skipped_wires = 0; inc_indent(); f << indent << "assert(path.empty() || path[path.size() - 1] == ' ');\n"; for (auto wire : module->wires()) { if (wire->name[0] != '\\') continue; - if (debug_alias_wires.count(wire)) { + if (debug_const_wires.count(wire)) { + // Wire tied to a constant + f << indent << "static const value<" << wire->width << "> const_" << mangle(wire) << " = "; + dump_const(debug_const_wires[wire]); + f << ";\n"; + f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); + f << ", debug_item(const_" << mangle(wire) << "));\n"; + count_const_wires++; + } else if (debug_alias_wires.count(wire)) { // Alias of a member wire f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); f << ", debug_item(" << mangle(debug_alias_wires[wire]) << "));\n"; count_alias_wires++; - continue; - } - if (!localized_wires.count(wire)) { + } else if (!localized_wires.count(wire)) { // Member wire f << indent << "items.emplace(path + " << escape_cxx_string(get_hdl_name(wire)); f << ", debug_item(" << mangle(wire) << "));\n"; count_member_wires++; - continue; + } else { + count_skipped_wires++; } - count_skipped_wires++; } for (auto &memory_it : module->memories) { if (memory_it.first[0] != '\\') @@ -1647,8 +1655,9 @@ struct CxxrtlWorker { dec_indent(); log_debug("Debug information statistics for module %s:\n", log_id(module)); - log_debug(" Member wires: %zu\n", count_member_wires); + log_debug(" Const wires: %zu\n", count_const_wires); log_debug(" Alias wires: %zu\n", count_alias_wires); + log_debug(" Member wires: %zu\n", count_member_wires); log_debug(" Other wires: %zu (no debug information)\n", count_skipped_wires); } @@ -2163,8 +2172,8 @@ struct CxxrtlWorker { eval_converges[module] = feedback_wires.empty() && buffered_wires.empty(); if (debug_info) { - // Find wires that alias other wires; debug information can be enriched with these at essentially zero - // additional cost. + // Find wires that alias other wires or are tied to a constant; debug information can be enriched with these + // at essentially zero additional cost. // // Note that the information collected here can't be used for optimizing the netlist: debug information queries // are pure and run on a design in a stable state, which allows assumptions that do not otherwise hold. @@ -2179,14 +2188,20 @@ struct CxxrtlWorker { break; // not an alias: complex def log_assert(flow.wire_comb_defs[wire_it].size() == 1); FlowGraph::Node *node = *flow.wire_comb_defs[wire_it].begin(); - if (node->connect.second.is_wire()) { - RTLIL::Wire *rhs_wire = node->connect.second.as_wire(); + if (node->type != FlowGraph::Node::Type::CONNECT) + break; // not an alias: def by cell + RTLIL::SigSpec rhs_sig = node->connect.second; + if (rhs_sig.is_wire()) { + RTLIL::Wire *rhs_wire = rhs_sig.as_wire(); if (localized_wires[rhs_wire]) { wire_it = rhs_wire; // maybe an alias } else { debug_alias_wires[wire] = rhs_wire; // is an alias break; } + } else if (rhs_sig.is_fully_const()) { + debug_const_wires[wire] = rhs_sig.as_const(); // is a const + break; } else { break; // not an alias: complex rhs } diff --git a/backends/cxxrtl/cxxrtl_capi.h b/backends/cxxrtl/cxxrtl_capi.h index bee5a94c7..46aa662b2 100644 --- a/backends/cxxrtl/cxxrtl_capi.h +++ b/backends/cxxrtl/cxxrtl_capi.h @@ -64,9 +64,10 @@ enum cxxrtl_type { // Values correspond to singly buffered netlist nodes, i.e. nodes driven exclusively by // combinatorial cells, or toplevel input nodes. // - // Values can be inspected via the `curr` pointer and modified via the `next` pointer (which are - // equal for values); however, note that changes to the bits driven by combinatorial cells will - // be ignored. + // Values can be inspected via the `curr` pointer. If the `next` pointer is NULL, the value is + // driven by a constant and can never be modified. Otherwise, the value can be modified through + // the `next` pointer (which is equal to `curr` if not NULL). Note that changes to the bits + // driven by combinatorial cells will be ignored. // // Values always have depth 1. CXXRTL_VALUE = 0, @@ -75,8 +76,8 @@ enum cxxrtl_type { // storage cells, or by combinatorial cells that are a part of a feedback path. // // Wires can be inspected via the `curr` pointer and modified via the `next` pointer (which are - // distinct for wires); however, note that changes to the bits driven by combinatorial cells will - // be ignored. + // distinct for wires). Note that changes to the bits driven by combinatorial cells will be + // ignored. // // Wires always have depth 1. CXXRTL_WIRE = 1, -- cgit v1.2.3 From 467152d79fd4c8e25b48481f742fe318b1395728 Mon Sep 17 00:00:00 2001 From: whitequark Date: Mon, 8 Jun 2020 17:38:11 +0000 Subject: cxxrtl: don't check immutable values for changes in VCD writer. This commit changes the VCD writer such that for all signals that have `debug_item.type == VALUE && debug_item.next == nullptr`, it would only sample the value once. Commit f2d7a187 added more debug information by including constant wires, and decreased the performance of VCD writer proportionally because the constant wires were still repeatedly sampled; this commit eliminates the performance hit. --- backends/cxxrtl/cxxrtl_vcd.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/backends/cxxrtl/cxxrtl_vcd.h b/backends/cxxrtl/cxxrtl_vcd.h index 8ba94ea77..5f5f612b5 100644 --- a/backends/cxxrtl/cxxrtl_vcd.h +++ b/backends/cxxrtl/cxxrtl_vcd.h @@ -104,19 +104,25 @@ class vcd_writer { buffer += '\n'; } - const variable ®ister_variable(size_t width, chunk_t *curr) { + const variable ®ister_variable(size_t width, chunk_t *curr, bool immutable = false) { if (aliases.count(curr)) { return variables[aliases[curr]]; } else { const size_t chunks = (width + (sizeof(chunk_t) * 8 - 1)) / (sizeof(chunk_t) * 8); aliases[curr] = variables.size(); - variables.emplace_back(variable { variables.size(), width, curr, cache.size() }); - cache.insert(cache.end(), &curr[0], &curr[chunks]); + if (immutable) { + variables.emplace_back(variable { variables.size(), width, curr, (size_t)-1 }); + } else { + variables.emplace_back(variable { variables.size(), width, curr, cache.size() }); + cache.insert(cache.end(), &curr[0], &curr[chunks]); + } return variables.back(); } } bool test_variable(const variable &var) { + if (var.prev_off == (size_t)-1) + return false; // immutable const size_t chunks = (var.width + (sizeof(chunk_t) * 8 - 1)) / (sizeof(chunk_t) * 8); if (std::equal(&var.curr[0], &var.curr[chunks], &cache[var.prev_off])) { return false; @@ -158,7 +164,7 @@ public: switch (item.type) { // Not the best naming but oh well... case debug_item::VALUE: - emit_var(register_variable(item.width, item.curr), "wire", name); + emit_var(register_variable(item.width, item.curr, /*immutable=*/item.next == nullptr), "wire", name); break; case debug_item::WIRE: emit_var(register_variable(item.width, item.curr), "reg", name); -- cgit v1.2.3