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.

diff --git a/backends/cxxrtl/cxxrtl.h b/backends/cxxrtl/cxxrtl.h
index eb7d7eaeb30a75527cb608f1b99404ffcaf0648d..d850fdba40b0dbe8202868d702d82c065f8bfa68 100644 (file)
--- a/backends/cxxrtl/cxxrtl.h
+++ b/backends/cxxrtl/cxxrtl.h
@@ -659,7 +659,7 @@ struct wire {
        value<Bits> next;
 
        wire() = default;
-       constexpr wire(const value<Bits> &init) : curr(init), next(init) {}
+       explicit constexpr wire(const value<Bits> &init) : curr(init), next(init) {}
        template<typename... Init>
        explicit constexpr wire(Init ...init) : curr{init...}, next{init...} {}
 

diff --git a/backends/cxxrtl/cxxrtl_backend.cc b/backends/cxxrtl/cxxrtl_backend.cc
index 7efb0aeaf3937cda1fd3a2410539000765f06765..c193d78e9157fcd75dd579115afe8e0063d7eda1 100644 (file)
--- 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";
+                       }
                }
        }