From e4aa8bc96b1650ef17d70a0ba7cd88b21b7e38dc Mon Sep 17 00:00:00 2001 From: whitequark Date: Fri, 11 Dec 2020 23:30:32 +0000 Subject: [PATCH] cxxrtl: don't overwrite buffered inputs. Before this commit, a cell's input was always assigned like: p_cell.p_input = (value...); If `p_input` is buffered (e.g. if the design is built at -O0), this is not correct. (In practice, this breaks clocking.) Unfortunately, the incorrect design was compiled without diagnostics because wire<> was move-assignable and also implicitly constructible from value<>. After this commit, cell inputs are no longer incorrectly assumed to always be unbuffered, and wires are not assignable from values. --- backends/cxxrtl/cxxrtl.h | 2 +- backends/cxxrtl/cxxrtl_backend.cc | 56 ++++++++++++++++++------------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/backends/cxxrtl/cxxrtl.h b/backends/cxxrtl/cxxrtl.h index eb7d7eaeb..d850fdba4 100644 --- a/backends/cxxrtl/cxxrtl.h +++ b/backends/cxxrtl/cxxrtl.h @@ -659,7 +659,7 @@ struct wire { value next; wire() = default; - constexpr wire(const value &init) : curr(init), next(init) {} + explicit constexpr wire(const value &init) : curr(init), next(init) {} template explicit constexpr wire(Init ...init) : curr{init...}, next{init...} {} diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc index 7efb0aeaf..c193d78e9 100644 --- a/backends/cxxrtl/cxxrtl_backend.cc +++ b/backends/cxxrtl/cxxrtl_backend.cc @@ -1240,10 +1240,19 @@ struct CxxrtlWorker { // User cells } else { log_assert(cell->known()); + bool buffered_inputs = false; const char *access = is_cxxrtl_blackbox_cell(cell) ? "->" : "."; for (auto conn : cell->connections()) - if (cell->input(conn.first) && !cell->output(conn.first)) { - f << indent << mangle(cell) << access << mangle_wire_name(conn.first) << " = "; + if (cell->input(conn.first)) { + RTLIL::Module *cell_module = cell->module->design->module(cell->type); + log_assert(cell_module != nullptr && cell_module->wire(conn.first) && conn.second.is_wire()); + RTLIL::Wire *cell_module_wire = cell_module->wire(conn.first); + f << indent << mangle(cell) << access << mangle_wire_name(conn.first); + if (!is_cxxrtl_blackbox_cell(cell) && !unbuffered_wires[cell_module_wire]) { + buffered_inputs = true; + f << ".next"; + } + f << " = "; dump_sigspec_rhs(conn.second); f << ";\n"; if (getenv("CXXRTL_VOID_MY_WARRANTY")) { @@ -1255,19 +1264,11 @@ struct CxxrtlWorker { // with: // top.prev_p_clk = value<1>{0u}; top.p_clk = value<1>{1u}; top.step(); // Don't rely on this; it will be removed without warning. - RTLIL::Module *cell_module = cell->module->design->module(cell->type); - if (cell_module != nullptr && cell_module->wire(conn.first) && conn.second.is_wire()) { - RTLIL::Wire *cell_module_wire = cell_module->wire(conn.first); - if (edge_wires[conn.second.as_wire()] && edge_wires[cell_module_wire]) { - f << indent << mangle(cell) << access << "prev_" << mangle(cell_module_wire) << " = "; - f << "prev_" << mangle(conn.second.as_wire()) << ";\n"; - } + if (edge_wires[conn.second.as_wire()] && edge_wires[cell_module_wire]) { + f << indent << mangle(cell) << access << "prev_" << mangle(cell_module_wire) << " = "; + f << "prev_" << mangle(conn.second.as_wire()) << ";\n"; } } - } else if (cell->input(conn.first)) { - f << indent << mangle(cell) << access << mangle_wire_name(conn.first) << ".next = "; - dump_sigspec_rhs(conn.second); - f << ";\n"; } auto assign_from_outputs = [&](bool cell_converged) { for (auto conn : cell->connections()) { @@ -1285,9 +1286,9 @@ struct CxxrtlWorker { // have any buffered wires if they were not output ports. Imagine inlining the cell's eval() function, // and consider the fate of the localized wires that used to be output ports.) // - // Unlike cell inputs (which are never buffered), it is not possible to know apriori whether the cell - // (which may be late bound) will converge immediately. Because of this, the choice between using .curr - // (appropriate for buffered outputs) and .next (appropriate for unbuffered outputs) is made at runtime. + // It is not possible to know apriori whether the cell (which may be late bound) will converge immediately. + // Because of this, the choice between using .curr (appropriate for buffered outputs) and .next (appropriate + // for unbuffered outputs) is made at runtime. if (cell_converged && is_cxxrtl_comb_port(cell, conn.first)) f << ".next;\n"; else @@ -1295,16 +1296,23 @@ struct CxxrtlWorker { } } }; - f << indent << "if (" << mangle(cell) << access << "eval()) {\n"; - inc_indent(); - assign_from_outputs(/*cell_converged=*/true); - dec_indent(); - f << indent << "} else {\n"; - inc_indent(); + 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 << "converged = false;\n"; assign_from_outputs(/*cell_converged=*/false); - dec_indent(); - f << indent << "}\n"; + } else { + f << indent << "if (" << mangle(cell) << access << "eval()) {\n"; + inc_indent(); + assign_from_outputs(/*cell_converged=*/true); + dec_indent(); + f << indent << "} else {\n"; + inc_indent(); + f << indent << "converged = false;\n"; + assign_from_outputs(/*cell_converged=*/false); + dec_indent(); + f << indent << "}\n"; + } } } -- 2.30.2