cxxrtl: Fix liveness checking for debug wires

[Lunaphied]

The current liveness checking for cxxrtl breaks in certain esoteric cases. Notably when signals that were completely optimized out get reintroduced into the design (so called "outline wires") those wires are wholly calculated inside the debug pass, this is because we want to keep the optimized code for the main eval pass and only do the full computation if the debug state is requested explicitly.

Due to some annoyances of yosys itself, it's actually somewhat difficult to have public wires in a design that are unused, as at some point earlier (likely the flatten pass?) yosys has essentially renamed the public signal into a internal unnamed signal, since cxxrtl only generates this additional debug information for used public signals (i.e. ones that are optimized out but not completely unused), this feature currently mostly goes to waste.

Issue #2540 managed to find a test case that actually triggered the generation of these "compute on demand" outline wires, revealing that it wasn't tagging all the proper wires as "live" and used during debug that it should have, causing asserts to fail while generating the debug pass. The details of the problem and proposed fix are explained further in a comment below.

[Lunaphied]

Currently, it seems that this fix can generate unused variables in the output source when on -g4 with a design that fails without the commit. I'm not positive what causes this and will have the follow the liveness checking logic in more detail to see where truely unused signals are getting reintroduced into the design.

[Lunaphied]

Background

After much effort I think I've fully determined the scope of the issue.

During the first pass of debug info generation, public wires are tagged with initial types to use during the debug path, however, when a wire is tagged as WireType::ALIAS this redirects to the wire it's connected to. Tagging with ALIAS occurs under two circumstances, the first is that the wire is a member wire (exposed in the generated c++ class, either because it's buffered or because it's an actual member or outline (debug only member)), this makes sense as these wires will be precomputed from the standard pass of eval().

yosys/backends/cxxrtl/cxxrtl_backend.cc

Lines 2600 to 2610 in 339848b

	debug_wire_type = {WireType::CONST, rhs}; // wire replaced with const
	} else if (rhs.is_wire()) {
	if (wire_types[rhs.as_wire()].is_member())
	debug_wire_type = {WireType::ALIAS, rhs}; // wire replaced with wire
	else if (debug_eval && rhs.as_wire()->name.isPublic())
	debug_wire_type = {WireType::ALIAS, rhs}; // wire replaced with outline
	it = rhs.as_wire(); // and keep looking
	continue;
	}
	break;
	}

The second condition is only done when debug eval is enabled (which is where this issue occurs i.e. -g4, currently the default), in this case the wire can be tagged as an alias as long as the name of the wire being aliased to is "public". Since all public wires will be exposed as outlines when debug_eval is enabled, we can also alias to these outline members.

The problem comes when we try to later use these aliases. Due to the way the code is structured, these aliased wires will be generated, but if they alias a member wire, it won't be tagged as "live". This is because we don't want to change non-debug state members in debug_eval, we just want to update the state that's added by allowing debug info to regenerate public signals that have been optimized away. Unfortunately skipping generating the nodes that update that state, also skips adding their connected wires to the debug_live_wires set as well.

Not being tagged as live means that they'll skip the second phase of assigning debug_wire_types unless some other reference makes that member "live". When this is skipped, the aliased member wires won't be marked as used for when the debug_eval method is being generated.

The Fix

My original fix was to consider all the wires that were live for the optimized eval() to be considered live for the debug pass, in addition to wires that were only live for debug. This seems to be the intent of the existing code, which seemingly had an accidental reversed condition (checking for liveness in both sets instead of either). This works, but some temporary (WireType::LOCAL) signals in the optimized pass get named in the debug pass, leading to unused variables in the final generated C++ file.

We shouldn't have unused variables though, since we know all the cells being generated and what their inputs and outputs are, this should be enough to only emit the used portions. So this solution isn't ideal.

The revised solution accomplishes this by also checking if the wire is a member variable from the optimized design and considering those variables to be live also. This will result in this logic being run:

yosys/backends/cxxrtl/cxxrtl_backend.cc

Lines 2668 to 2671 in 339848b

	} else {
	log_assert(wire_type.is_member());
	debug_wire_type = wire_type; // wire is a member
	}

Which will accomplish the task of assigning member variables in the design as "used" by debug, but no more. This gets rid of the extra locals and seems to completely solve the problem.

[mwkmwkmwk]

I'm sorry I don't have the expertise to review your PR (that will have to wait for @whitequark), but please write a better header line than "fix #bugnumber" and rename the PR appropriately — something like "cxxrtl: fix liveness checking for debug wires" would be perfect.

[Lunaphied]

Sorry about that, at the time I was trying to focus the discussion down into the issue itself since at first I had written an explanation there and thought it was going to be a quick simple patch. It ends up that it's not quite so straightforward and I never updated it later.

I see you updated the title for me already, so I went ahead and gave a more meaningful summary in the description too.

[whitequark]

Superseded by #2871.