Merge pull request #2371 from whitequark/cxxrtl-debug-info

cxxrtl: expose port direction and driver kind in debug information

backends/cxxrtl/cxxrtl.h

@@ -452,7 +452,7 @@ struct value : public expr_base<value<Bits>> {
 		bool carry = CarryIn;
 		for (size_t n = 0; n < result.chunks; n++) {
 			result.data[n] = data[n] + (Invert ? ~other.data[n] : other.data[n]) + carry;
-			if (result.chunks - 1 == n) 
+			if (result.chunks - 1 == n)
 				result.data[result.chunks - 1] &= result.msb_mask;
 			carry = (result.data[n] <  data[n]) ||
 			        (result.data[n] == data[n] && carry);
@@ -824,6 +824,7 @@ struct debug_alias {};
 // To avoid violating strict aliasing rules, this structure has to be a subclass of the one used
 // in the C API, or it would not be possible to cast between the pointers to these.
 struct debug_item : ::cxxrtl_object {
+	// Object types.
 	enum : uint32_t {
 		VALUE  = CXXRTL_VALUE,
 		WIRE   = CXXRTL_WIRE,
@@ -831,13 +832,24 @@ struct debug_item : ::cxxrtl_object {
 		ALIAS  = CXXRTL_ALIAS,
 	};
 
+	// Object flags.
+	enum : uint32_t {
+		INPUT  = CXXRTL_INPUT,
+		OUTPUT = CXXRTL_OUTPUT,
+		INOUT  = CXXRTL_INOUT,
+		DRIVEN_SYNC = CXXRTL_DRIVEN_SYNC,
+		DRIVEN_COMB = CXXRTL_DRIVEN_COMB,
+		UNDRIVEN    = CXXRTL_UNDRIVEN,
+	};
+
 	debug_item(const ::cxxrtl_object &object) : cxxrtl_object(object) {}
 
 	template<size_t Bits>
-	debug_item(value<Bits> &item, size_t lsb_offset = 0) {
+	debug_item(value<Bits> &item, size_t lsb_offset = 0, uint32_t flags_ = 0) {
 		static_assert(sizeof(item) == value<Bits>::chunks * sizeof(chunk_t),
 		              "value<Bits> is not compatible with C layout");
 		type    = VALUE;
+		flags   = flags_;
 		width   = Bits;
 		lsb_at  = lsb_offset;
 		depth   = 1;
@@ -851,6 +863,7 @@ struct debug_item : ::cxxrtl_object {
 		static_assert(sizeof(item) == value<Bits>::chunks * sizeof(chunk_t),
 		              "value<Bits> is not compatible with C layout");
 		type    = VALUE;
+		flags   = DRIVEN_COMB;
 		width   = Bits;
 		lsb_at  = lsb_offset;
 		depth   = 1;
@@ -860,11 +873,12 @@ struct debug_item : ::cxxrtl_object {
 	}
 
 	template<size_t Bits>
-	debug_item(wire<Bits> &item, size_t lsb_offset = 0) {
+	debug_item(wire<Bits> &item, size_t lsb_offset = 0, uint32_t flags_ = 0) {
 		static_assert(sizeof(item.curr) == value<Bits>::chunks * sizeof(chunk_t) &&
 		              sizeof(item.next) == value<Bits>::chunks * sizeof(chunk_t),
 		              "wire<Bits> is not compatible with C layout");
 		type    = WIRE;
+		flags   = flags_;
 		width   = Bits;
 		lsb_at  = lsb_offset;
 		depth   = 1;
@@ -878,6 +892,7 @@ struct debug_item : ::cxxrtl_object {
 		static_assert(sizeof(item.data[0]) == value<Width>::chunks * sizeof(chunk_t),
 		              "memory<Width> is not compatible with C layout");
 		type    = MEMORY;
+		flags   = 0;
 		width   = Width;
 		lsb_at  = 0;
 		depth   = item.data.size();
@@ -891,6 +906,7 @@ struct debug_item : ::cxxrtl_object {
 		static_assert(sizeof(item) == value<Bits>::chunks * sizeof(chunk_t),
 		              "value<Bits> is not compatible with C layout");
 		type    = ALIAS;
+		flags   = DRIVEN_COMB;
 		width   = Bits;
 		lsb_at  = lsb_offset;
 		depth   = 1;
@@ -905,6 +921,7 @@ struct debug_item : ::cxxrtl_object {
 		              sizeof(item.next) == value<Bits>::chunks * sizeof(chunk_t),
 		              "wire<Bits> is not compatible with C layout");
 		type    = ALIAS;
+		flags   = DRIVEN_COMB;
 		width   = Bits;
 		lsb_at  = lsb_offset;
 		depth   = 1;

backends/cxxrtl/cxxrtl_backend.cc

@@ -200,16 +200,12 @@ bool is_elidable_cell(RTLIL::IdString type)
 		ID($mux), ID($concat), ID($slice), ID($pmux));
 }
 
-bool is_sync_ff_cell(RTLIL::IdString type)
-{
-	return type.in(
-		ID($dff), ID($dffe), ID($sdff), ID($sdffe), ID($sdffce));
-}
-
 bool is_ff_cell(RTLIL::IdString type)
 {
-	return is_sync_ff_cell(type) || type.in(
+	return type.in(
-		ID($adff), ID($adffe), ID($dffsr), ID($dffsre), ID($dlatch), ID($adlatch), ID($dlatchsr), ID($sr));
+		ID($dff), ID($dffe), ID($sdff), ID($sdffe), ID($sdffce),
+		ID($adff), ID($adffe), ID($dffsr), ID($dffsre),
+		ID($dlatch), ID($adlatch), ID($dlatchsr), ID($sr));
 }
 
 bool is_internal_cell(RTLIL::IdString type)
@@ -277,6 +273,7 @@ struct FlowGraph {
 	std::vector<Node*> nodes;
 	dict<const RTLIL::Wire*, pool<Node*, hash_ptr_ops>> wire_comb_defs, wire_sync_defs, wire_uses;
 	dict<const RTLIL::Wire*, bool> wire_def_elidable, wire_use_elidable;
+	dict<RTLIL::SigBit, bool> bit_has_state;
 
 	~FlowGraph()
 	{
@@ -284,17 +281,24 @@ struct FlowGraph {
 			delete node;
 	}
 
-	void add_defs(Node *node, const RTLIL::SigSpec &sig, bool fully_sync, bool elidable)
+	void add_defs(Node *node, const RTLIL::SigSpec &sig, bool is_ff, bool elidable)
 	{
 		for (auto chunk : sig.chunks())
 			if (chunk.wire) {
-				if (fully_sync)
+				if (is_ff) {
+					// A sync def means that a wire holds design state because it is driven directly by
+					// a flip-flop output. Such a wire can never be unbuffered.
 					wire_sync_defs[chunk.wire].insert(node);
-				else
+				} else {
+					// A comb def means that a wire doesn't hold design state. It might still be connected,
+					// indirectly, to a flip-flop output.
 					wire_comb_defs[chunk.wire].insert(node);
+				}
 			}
+		for (auto bit : sig.bits())
+			bit_has_state[bit] |= is_ff;
 		// Only comb defs of an entire wire in the right order can be elided.
-		if (!fully_sync && sig.is_wire())
+		if (!is_ff && sig.is_wire())
 			wire_def_elidable[sig.as_wire()] = elidable;
 	}
 
@@ -322,7 +326,7 @@ struct FlowGraph {
 	// Connections
 	void add_connect_defs_uses(Node *node, const RTLIL::SigSig &conn)
 	{
-		add_defs(node, conn.first, /*fully_sync=*/false, /*elidable=*/true);
+		add_defs(node, conn.first, /*is_ff=*/false, /*elidable=*/true);
 		add_uses(node, conn.second);
 	}
 
@@ -369,7 +373,7 @@ struct FlowGraph {
 			if (cell->output(conn.first))
 				if (is_cxxrtl_sync_port(cell, conn.first)) {
 					// See note regarding elidability below.
-					add_defs(node, conn.second, /*fully_sync=*/false, /*elidable=*/false);
+					add_defs(node, conn.second, /*is_ff=*/false, /*elidable=*/false);
 				}
 	}
 
@@ -378,18 +382,18 @@ struct FlowGraph {
 		for (auto conn : cell->connections()) {
 			if (cell->output(conn.first)) {
 				if (is_elidable_cell(cell->type))
-					add_defs(node, conn.second, /*fully_sync=*/false, /*elidable=*/true);
+					add_defs(node, conn.second, /*is_ff=*/false, /*elidable=*/true);
-				else if (is_sync_ff_cell(cell->type) || (cell->type == ID($memrd) && cell->getParam(ID::CLK_ENABLE).as_bool()))
+				else if (is_ff_cell(cell->type) || (cell->type == ID($memrd) && cell->getParam(ID::CLK_ENABLE).as_bool()))
-					add_defs(node, conn.second, /*fully_sync=*/true,  /*elidable=*/false);
+					add_defs(node, conn.second, /*is_ff=*/true,  /*elidable=*/false);
 				else if (is_internal_cell(cell->type))
-					add_defs(node, conn.second, /*fully_sync=*/false, /*elidable=*/false);
+					add_defs(node, conn.second, /*is_ff=*/false, /*elidable=*/false);
 				else if (!is_cxxrtl_sync_port(cell, conn.first)) {
 					// Although at first it looks like outputs of user-defined cells may always be elided, the reality is
 					// more complex. Fully sync outputs produce no defs and so don't participate in elision. Fully comb
 					// outputs are assigned in a different way depending on whether the cell's eval() immediately converged.
 					// Unknown/mixed outputs could be elided, but should be rare in practical designs and don't justify
 					// the infrastructure required to elide outputs of cells with many of them.
-					add_defs(node, conn.second, /*fully_sync=*/false, /*elidable=*/false);
+					add_defs(node, conn.second, /*is_ff=*/false, /*elidable=*/false);
 				}
 			}
 			if (cell->input(conn.first))
@@ -427,7 +431,7 @@ struct FlowGraph {
 	void add_case_defs_uses(Node *node, const RTLIL::CaseRule *case_)
 	{
 		for (auto &action : case_->actions) {
-			add_defs(node, action.first, /*is_sync=*/false, /*elidable=*/false);
+			add_defs(node, action.first, /*is_ff=*/false, /*elidable=*/false);
 			add_uses(node, action.second);
 		}
 		for (auto sub_switch : case_->switches) {
@@ -446,9 +450,9 @@ struct FlowGraph {
 		for (auto sync : process->syncs)
 			for (auto action : sync->actions) {
 				if (sync->type == RTLIL::STp || sync->type == RTLIL::STn || sync->type == RTLIL::STe)
-				  add_defs(node, action.first, /*is_sync=*/true,  /*elidable=*/false);
+				  add_defs(node, action.first, /*is_ff=*/true,  /*elidable=*/false);
 				else
-					add_defs(node, action.first, /*is_sync=*/false, /*elidable=*/false);
+					add_defs(node, action.first, /*is_ff=*/false, /*elidable=*/false);
 				add_uses(node, action.second);
 			}
 	}
@@ -549,6 +553,7 @@ struct CxxrtlWorker {
 	pool<const RTLIL::Wire*> localized_wires;
 	dict<const RTLIL::Wire*, const RTLIL::Wire*> debug_alias_wires;
 	dict<const RTLIL::Wire*, RTLIL::Const> debug_const_wires;
+	dict<RTLIL::SigBit, bool> bit_has_state;
 	dict<const RTLIL::Module*, pool<std::string>> blackbox_specializations;
 	dict<const RTLIL::Module*, bool> eval_converges;
 
@@ -1142,7 +1147,7 @@ struct CxxrtlWorker {
 				}
 				// The generated code has two bounds checks; one in an assertion, and another that guards the read.
 				// This is done so that the code does not invoke undefined behavior under any conditions, but nevertheless
-				// loudly crashes if an illegal condition is encountered. The assert may be turned off with -NDEBUG not
+				// loudly crashes if an illegal condition is encountered. The assert may be turned off with -DNDEBUG not
 				// just for release builds, but also to make sure the simulator (which is presumably embedded in some
 				// larger program) will never crash the code that calls into it.
 				//
@@ -1635,6 +1640,10 @@ struct CxxrtlWorker {
 		size_t count_alias_wires = 0;
 		size_t count_member_wires = 0;
 		size_t count_skipped_wires = 0;
+		size_t count_driven_sync = 0;
+		size_t count_driven_comb = 0;
+		size_t count_undriven = 0;
+		size_t count_mixed_driver = 0;
 		inc_indent();
 			f << indent << "assert(path.empty() || path[path.size() - 1] == ' ');\n";
 			for (auto wire : module->wires()) {
@@ -1660,9 +1669,55 @@ struct CxxrtlWorker {
 					count_alias_wires++;
 				} else if (!localized_wires.count(wire)) {
 					// Member wire
+					std::vector<std::string> flags;
+
+					if (wire->port_input && wire->port_output)
+						flags.push_back("INOUT");
+					else if (wire->port_input)
+						flags.push_back("INPUT");
+					else if (wire->port_output)
+						flags.push_back("OUTPUT");
+
+					bool has_driven_sync = false;
+					bool has_driven_comb = false;
+					bool has_undriven = false;
+					SigSpec sig(wire);
+					for (auto bit : sig.bits())
+						if (!bit_has_state.count(bit))
+							has_undriven = true;
+						else if (bit_has_state[bit])
+							has_driven_sync = true;
+						else
+							has_driven_comb = true;
+					if (has_driven_sync)
+						flags.push_back("DRIVEN_SYNC");
+					if (has_driven_sync && !has_driven_comb && !has_undriven)
+						count_driven_sync++;
+					if (has_driven_comb)
+						flags.push_back("DRIVEN_COMB");
+					if (!has_driven_sync && has_driven_comb && !has_undriven)
+						count_driven_comb++;
+					if (has_undriven)
+						flags.push_back("UNDRIVEN");
+					if (!has_driven_sync && !has_driven_comb && has_undriven)
+						count_undriven++;
+					if (has_driven_sync + has_driven_comb + has_undriven > 1)
+						count_mixed_driver++;
+
 					f << indent << "items.add(path + " << escape_cxx_string(get_hdl_name(wire));
 					f << ", debug_item(" << mangle(wire) << ", ";
-					f << wire->start_offset << "));\n";
+					f << wire->start_offset;
+					bool first = true;
+					for (auto flag : flags) {
+						if (first) {
+							first = false;
+							f << ", ";
+						} else {
+							f << "|";
+						}
+						f << "debug_item::" << flag;
+					}
+					f << "));\n";
 					count_member_wires++;
 				} else {
 					count_skipped_wires++;
@@ -1690,7 +1745,11 @@ struct CxxrtlWorker {
 		log_debug("  Public wires: %zu, of which:\n", count_public_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("    Member wires: %zu, of which:\n", count_member_wires);
+		log_debug("      Driven sync:  %zu\n", count_driven_sync);
+		log_debug("      Driven comb:  %zu\n", count_driven_comb);
+		log_debug("      Undriven:     %zu\n", count_undriven);
+		log_debug("      Mixed driver: %zu\n", count_mixed_driver);
 		log_debug("    Other wires:  %zu (no debug information)\n", count_skipped_wires);
 	}
 
@@ -2209,6 +2268,9 @@ struct CxxrtlWorker {
 
 			eval_converges[module] = feedback_wires.empty() && buffered_comb_wires.empty();
 
+			for (auto item : flow.bit_has_state)
+				bit_has_state.insert(item);
+
 			if (debug_info) {
 				// Find wires that alias other wires or are tied to a constant; debug information can be enriched with these
 				// at essentially zero additional cost.

backends/cxxrtl/cxxrtl_capi.h

@@ -73,6 +73,10 @@ int cxxrtl_commit(cxxrtl_handle handle);
 size_t cxxrtl_step(cxxrtl_handle handle);
 
 // Type of a simulated object.
+//
+// The type of a simulated object indicates the way it is stored and the operations that are legal
+// to perform on it (i.e. won't crash the simulation). It says very little about object semantics,
+// which is specified through flags.
 enum cxxrtl_type {
 	// Values correspond to singly buffered netlist nodes, i.e. nodes driven exclusively by
 	// combinatorial cells, or toplevel input nodes.
@@ -86,7 +90,8 @@ enum cxxrtl_type {
 	CXXRTL_VALUE = 0,
 
 	// Wires correspond to doubly buffered netlist nodes, i.e. nodes driven, at least in part, by
-	// storage cells, or by combinatorial cells that are a part of a feedback path.
+	// storage cells, or by combinatorial cells that are a part of a feedback path. They are also
+	// present in non-optimized builds.
 	//
 	// Wires can be inspected via the `curr` pointer and modified via the `next` pointer (which are
 	// distinct for wires). Note that changes to the bits driven by combinatorial cells will be
@@ -103,7 +108,7 @@ enum cxxrtl_type {
 	CXXRTL_MEMORY = 2,
 
 	// Aliases correspond to netlist nodes driven by another node such that their value is always
-	// exactly equal, or driven by a constant value.
+	// exactly equal.
 	//
 	// Aliases can be inspected via the `curr` pointer. They cannot be modified, and the `next`
 	// pointer is always NULL.
@@ -112,6 +117,66 @@ enum cxxrtl_type {
 	// More object types may be added in the future, but the existing ones will never change.
 };
 
+// Flags of a simulated object.
+//
+// The flags of a simulated object indicate its role in the netlist:
+//  * The flags `CXXRTL_INPUT` and `CXXRTL_OUTPUT` designate module ports.
+//  * The flags `CXXRTL_DRIVEN_SYNC`, `CXXRTL_DRIVEN_COMB`, and `CXXRTL_UNDRIVEN` specify
+//    the semantics of node state. An object with several of these flags set has different bits
+//    follow different semantics.
+enum cxxrtl_flag {
+	// Node is a module input port.
+	//
+	// This flag can be set on objects of type `CXXRTL_VALUE` and `CXXRTL_WIRE`. It may be combined
+	// with `CXXRTL_OUTPUT`, as well as other flags.
+	CXXRTL_INPUT = 1 << 0,
+
+	// Node is a module output port.
+	//
+	// This flag can be set on objects of type `CXXRTL_WIRE`. It may be combined with `CXXRTL_INPUT`,
+	// as well as other flags.
+	CXXRTL_OUTPUT = 1 << 1,
+
+	// Node is a module inout port.
+	//
+	// This flag can be set on objects of type `CXXRTL_WIRE`. It may be combined with other flags.
+	CXXRTL_INOUT = (CXXRTL_INPUT|CXXRTL_OUTPUT),
+
+	// Node has bits that are driven by a storage cell.
+	//
+	// This flag can be set on objects of type `CXXRTL_WIRE`. It may be combined with
+	// `CXXRTL_DRIVEN_COMB` and `CXXRTL_UNDRIVEN`, as well as other flags.
+	//
+	// This flag is set on wires that have bits connected directly to the output of a flip-flop or
+	// a latch, and hold its state. Many `CXXRTL_WIRE` objects may not have the `CXXRTL_DRIVEN_SYNC`
+	// flag set; for example, output ports and feedback wires generally won't. Writing to the `next`
+	// pointer of these wires updates stored state, and for designs without combinatorial loops,
+	// capturing the value from every of these wires through the `curr` pointer creates a complete
+	// snapshot of the design state.
+	CXXRTL_DRIVEN_SYNC = 1 << 2,
+
+	// Node has bits that are driven by a combinatorial cell or another node.
+	//
+	// This flag can be set on objects of type `CXXRTL_VALUE` and `CXXRTL_WIRE`. It may be combined
+	// with `CXXRTL_DRIVEN_SYNC` and `CXXRTL_UNDRIVEN`, as well as other flags.
+	//
+	// This flag is set on objects that have bits connected to the output of a combinatorial cell,
+	// or directly to another node. For designs without combinatorial loops, writing to such bits
+	// through the `next` pointer (if it is not NULL) has no effect.
+	CXXRTL_DRIVEN_COMB = 1 << 3,
+
+	// Node has bits that are not driven.
+	//
+	// This flag can be set on objects of type `CXXRTL_VALUE` and `CXXRTL_WIRE`. It may be combined
+	// with `CXXRTL_DRIVEN_SYNC` and `CXXRTL_DRIVEN_COMB`, as well as other flags.
+	//
+	// This flag is set on objects that have bits not driven by an output of any cell or by another
+	// node, such as inputs and dangling wires.
+	CXXRTL_UNDRIVEN = 1 << 4,
+
+	// More object flags may be added in the future, but the existing ones will never change.
+};
+
 // Description of a simulated object.
 //
 // The `data` array can be accessed directly to inspect and, if applicable, modify the bits
@@ -123,6 +188,9 @@ struct cxxrtl_object {
 	// determines all other properties of the object.
 	uint32_t type; // actually `enum cxxrtl_type`
 
+	// Flags of the object.
+	uint32_t flags; // actually bit mask of `enum cxxrtl_flags`
+
 	// Width of the object in bits.
 	size_t width;