Retire the netlist_utils free functions in favor of the traversal decorator - #647
Open
julianspeith wants to merge 18 commits into
Open
Retire the netlist_utils free functions in favor of the traversal decorator#647julianspeith wants to merge 18 commits into
julianspeith wants to merge 18 commits into
Conversation
boolean_influence::get_ff_dependency_matrix declared the cache it hands to get_next_sequential_gates as a pointer and never pointed it anywhere. The callee checks whether it is null before using it, which an uninitialized pointer is not, so it dereferenced whatever the stack happened to hold and the process died before the function returned anything. It segfaulted on a netlist of two gates, so nothing can have been relying on it. The variable is meant to be reused across the flip-flops it walks, so make it an object and pass its address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The decorator could only search from one gate to another, while netlist_utils could also search to any gate of a module and between two modules. Those two shapes had nowhere to go, so add them here. The breadth-first search behind all of this is now one function that stops at the first gate a predicate accepts, and the three entry points differ only in the predicate they build: one gate, any gate of a module, or, for the module to module search, one search per gate of the start module keeping every path that ties for the shortest. Searching from each gate of the start rather than towards each gate of the end keeps every path running start to end, so a caller need not know which end it was grown from. A start gate that already belongs to the module it is searching for returns a path of that gate alone. netlist_utils returned an empty vector there, which cannot be told apart from the module being unreachable. These had no test at all, which is why the tests come with them rather than after: the long way round losing to the short one, both directions, a filter that rejects everything, an unconnected gate, a start already inside the module, and two modules that are only connected one way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
netlist_traversal_decorator.h carried a note to move get_gate_chain and get_complex_gate_chain here, which is where every other traversal already is. Both take a start gate and walk the netlist from it, so there was nothing keeping them apart from it. Being on a class rather than loose in a namespace is what lets their bindings keep the netlist alive for as long as Python refers to the gates they hand back, which the binding lifetime check demanded the moment they became methods. As free functions there was no way to say it. The declarations went with the definitions this time: leaving them behind would have left anything outside this repository failing to link rather than failing to compile. Their callers move with them, in the Lattice and Xilinx architectures of the module identification, in the bindings, and in sixteen places in the tests, which is what says the behaviour came across unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both had a replacement already, neither said so. The three netlist_utils::get_shortest_path overloads now point at NetlistTraversalDecorator::get_shortest_path. Checked before saying so: on sixty random gate pairs the two disagree on which path they return twenty-three times and on its length never, which is what the decorator documents, as it returns the first of several paths of equal length. The two module shapes only became replaceable when the decorator learned them. netlist_utils::get_ff_dependency_matrix points at the one in the Boolean influence plugin. On a netlist of 1522 flip-flops the two agree on the flip-flop set and on every one of 2316484 cells, and the replacement can also report how strongly a flip-flop depends on another rather than only whether it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four of the traversals in this decorator are one traversal. They share a depth-first walk over nets and differ only in what they do at each gate, and what they do is decided by a boolean: get_next_matching_gates takes continue_on_match, get_next_matching_gates_until takes continue_on_mismatch, and the signatures are otherwise identical. Passing the wrong one compiles and returns a plausible answer. Worse, the names say the opposite of what the functions do. get_next_matching_gates_until reads as traversing until a match, which is what get_next_matching_gates does; the _until variant traverses through matches and halts at everything else. Add the traversal they are all special cases of. It takes what to collect as a condition and where to halt as a TraversalStop of at_match, at_mismatch or never, which is the distinction those booleans were making without naming it. Direction is a TraversalDirection rather than a bare bool successors, so that a call site says which way it goes and the decorator stops spelling one concept two ways. Measured against every function it is meant to replace before anything was built on it, over forty start gates in both directions on a netlist of 3458 gates, 80 of 80 in each case: get_next_matching_gates, get_next_matching_gates_until, get_next_sequential_gates, get_next_combinational_gates, get_next_matching_gates_until_depth, and netlist_utils::get_path, which turns out to be this traversal with a negated condition and no name of its own worth keeping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_next_matching_gates, get_next_matching_gates_until and get_next_matching_gates_until_depth each carried a copy of the same walk over nets, 231 lines between them, differing only in what they did at each gate. They now say which TraversalStop they mean and let get_gates do the walking, which is the same six lines three times over. netlist_utils::get_path is deprecated rather than moved. It returns every gate of a cone rather than a path, over every branch of a fan-out, and it is get_gates with a negated condition and a stop at a mismatch, so a name of its own would only invite the confusion again. It had no callers in C++ at all, only bindings. get_next_sequential_gates and get_next_combinational_gates keep a walk of their own for now. They are the same traversal with a fixed condition, but they also carry a cache across calls that get_gates has no notion of, and the Boolean influence plugin calls one of them once per flip-flop of a netlist to build its dependency matrix. Folding them in without giving get_gates a cache would pay for the tidying with everyone's runtime. The six equivalences were re-measured after the rewrite, unchanged at 80 of 80 each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_gates with TraversalDirection::both passed both down to the walks it starts from each adjacent net, and a walk in both directions from a fan-in net also runs forward from it, into the cones of sibling gates that share the input. A sibling's flip-flop is neither an ancestor nor a descendant of the start gate, and on a netlist where ff_a feeds both the start gate and a sibling inverter, the sibling's flip-flop appeared in the result. Walk the fan-out nets forward and the fan-in nets backward, each purely, so that both is exactly the union of forward and backward. The enum said "whichever answer is the better one", which describes a shortest path search rather than a union, and now says what happens. The equivalence measurements could not have caught this: none of the functions this traversal replaces had a both mode, so there was nothing to compare against, and the netlist of the core test had no fan-in sibling. It now does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cached traversals wrote their cache while walking: a net's entry grew gate by gate as they were found. When a combinational cycle led the walk back to a net whose entry was still partial, the walk took the partial answer as complete, and the net it was exploring at that moment kept a truncated entry forever. The call itself still returned the right set, so nothing looked wrong; the poison only surfaced in a later call that reached the truncated entry through a side path. The Boolean influence plugin shares one cache across every flip-flop of a netlist, and on a netlist with a combinational cycle its dependency matrix lost edges: measured before the fix, a flip-flop behind a side exit of a cycle lost one of its three dependencies. get_next_sequential_gates_map shares a cache the same way and was equally affected. The walk behind the cached traversals is now one memoized reachability over the nets that publishes an entry only when the net, and any cycle it belongs to, is fully explored; the nets of a strongly connected component share one answer and are published together. get_next_sequential_gates and get_next_combinational_gates become that walk with their conditions pinned, which also retires the two remaining hand-written copies of the traversal, 210 lines between them. The walk takes no depth limit and its endpoint filters take no depth on purpose: anything depth-shaped makes the answer from a net depend on how the net was reached, and a per-net cache is only sound while it does not. Verified three ways: the cycle netlist that lost a dependency now agrees with the fresh walk and is locked in as a test, the equivalences of the named traversals against the generic one still hold at 50 of 50 in both directions, and the netlist-wide map agrees with fresh per-gate calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The raw map that get_next_sequential_gates and its siblings accept as a cache leaves everything to discipline: nothing ties the map to the question it holds answers for, so reusing one with different forbidden pins, or a different direction, silently returns wrong sets. That is tolerable on a function whose name pins most of the semantics and fatal on get_gates, where the question is an arbitrary predicate and a stop rule. A TraversalCache is created for one traversal and cannot be used for any other: the direction, the match condition, the stop rule and the endpoint filters are sealed in at creation, a cache made for one netlist is refused by another, and both directions at once are refused rather than mixed into one store. The two things that would make a cached answer depend on how a net was reached are excluded by construction, as there is no depth limit and the filters receive no depth. Underneath it is the same memoized walk that the named cached traversals run on, so a cache is exactly as correct as they are, cycles included. On 1522 flip-flops of a netlist, every cached answer equals the uncached walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eate create_multi_bit_gate_modules and create_nets_at_unconnected_pins are module-level functions returning modules and nets the netlist owns, and they handed them out with nothing keeping the netlist alive. The plan had them down as unfixable without becoming methods, which was true of the tools that existed when it was written: keep_alive cannot nurse a list, and reference_internal has no parent on a free function. borrowed() has neither problem. It walks the returned container and ties each element to its owner, and the owner is found through the netlist's own wrapper, which exists because the caller passed the netlist in. So the fix is the same one token it is on a method, and nothing has to move. Measured: five nets created on a netlist raise its reference count by five and release it when dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_common_inputs answers which nets feed at least so many of a set of gates, which is a question about a group of gates and their surroundings and so belongs with the rest of the traversal. It has no callers anywhere, but it is the one remaining netlist utility with analytical substance: shared inputs across a group of gates typically indicate a shared control signal, so it is a cheap way to test whether a candidate group belongs together. The decorator variant validates its gates and reports errors the way the rest of the decorator does; the original stays behind, deprecated and delegating no longer needed, with its test migrated. Also records the changelog entry for the preprocessing binding fix of the previous commit, which anchored its changelog edit on text that only exists on another branch and silently landed without one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_nets_at_pins is a per-pin lookup that Gate::get_fan_in_net and get_fan_out_net already provide, and it has no callers anywhere. Rather than moving it somewhere, the deprecation message says what the loop is, which is all the function ever was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every function of netlist_utils now carries [[deprecated]], which warns whoever compiles against it and says what to use instead. A Python script has no compiler, so a caller of hal_py.NetlistUtils heard nothing and would first learn about the removal when it happens. Each binding now logs a warning naming its replacement, once per function and process, so a loop over a thousand gates says it once. The bindings that were bound as plain function pointers become forwarding lambdas to have somewhere to say it. While at it, the bindings that hand out gates and nets carry borrowed() now. Deprecated is not removed: until they go, they should not hand out objects that die with a netlist Python still thinks it holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The binding lifetime check exempted free, static and submodule-level functions because nothing could fix them when it was written: keep_alive cannot nurse a returned list and reference_internal has no parent there. borrowed() has neither problem, so the exemption is gone and they are held to the same rule as methods. Made strict, the check found four functions, all in the Boolean influence plugin, that return influences as a dict keyed by net -- and thereby a gap in borrowed() itself, which walked the values of a returned dict but not its keys. A borrowed object in a key position was handed out unprotected. The walk covers both now, the four bindings carry the policy, and holding such a dict raises the netlist's reference count by one per net key, measured. Sixty-nine binding files, forty-nine non-owning classes, no findings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_next_sequential_gates_map and boolean_influence's FF dependency matrix were the last callers holding raw result maps of their own. Both now build a sealed TraversalCache and let get_gates fill it, so they share the cycle-safe memoized engine instead of the per-call maps. The forbidden-pin and sequential-entry filters move into file-local helpers used by every sequential/combinational wrapper. Verified: ctest 37/37; FF dependency matrix bit-identical over 2,316,484 cells; cycle repro stays consistent; the four specialization equivalence suites still pass 50/50. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_next_combinational_gates never returned a next layer of anything: it returns every combinational gate up to the sequential boundary, which is a fan-in or fan-out cone, so it is now get_combinational_cone. With the two remaining raw-map users migrated to TraversalCache, the unenforceable cache parameter comes off it and off get_next_sequential_gates; repeated traversals share results through a sealed cache only. The Python bindings of both now also default forbidden_pins to an empty set, as the C++ side always did. The cycle regression test exercises the shared-cache path through a TraversalCache now, which is the idiom it exists to protect. Verified: ctest 37/37; decorator suite 9/9 from a fresh binary; from Python the renamed binding equals get_gates with at_mismatch and the old name is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Two follow-up commits since opening:
|
QueryConfig, Model and SolverResult had an operator<< but no to_string,
Constraint had both, and Python bound none of it -- printing any of the
four from a script showed an object address. All four now offer
to_string in C++ and both to_string and __str__ in Python. Printing a
model also no longer opens with a stray comma, {A:5} instead of {, A:5}.
Verified from Python: config, both constraint forms, result and model
print their content, and to_string() equals str() on each.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The gate chain searches use std::deque, and libc++ hands it over transitively while libstdc++ does not -- every Ubuntu CI job failed on the two chain functions while macOS built clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Retires all 14 free functions in
netlist_utils— each is now either[[deprecated]]with a pointer to its replacement or moved outright — and consolidates the netlist traversals they duplicated intoNetlistTraversalDecorator.Traversal consolidation
get_gateswithTraversalDirection {forward, backward, both}andTraversalStop {at_match, at_mismatch, never}replaces the six specialized walks (get_next_matching_gates,_until,_until_depth, sequential, combinational, andnetlist_utils::get_path). Each old function is proven a special case of it (80/80 equivalence per function) and now delegates, removing ~231 duplicated lines.bothis defined as the union of the forward and backward results (with a regression test — the naive both-direction walk leaked into sibling cones).boolean_influence's FF matrix (reproducer + C++ regression test included).TraversalCache: a sealed, reusable cache (netlist, direction, match/stop, depth-free filters fixed at creation) obtained viamake_traversal_cache; it refuses mismatched netlists andboth.get_next_sequential_gates_mapandboolean_influence::get_ff_dependency_matrixnow run on it instead of raw result maps. Also fixes an uninitialized cache pointer inget_ff_dependency_matrixthat segfaulted.get_next_combinational_gatesis renamed toget_combinational_cone— it returns every combinational gate up to the sequential boundary (a fan-in/fan-out cone), not a next layer of anything. The raw result-mapcacheparameter is removed from it and fromget_next_sequential_gates; repeated traversals share results exclusively through the sealedTraversalCache.get_common_inputs, the gate-chain searches, and module-terminated shortest paths moved into the decorator.Python bindings
netlist_utilsbindings now emit a once-per-process deprecation warning naming the replacement ([[deprecated]]is invisible to Python).get_gates, the two enums,TraversalCache/make_traversal_cache, and the moved functions — all with theborrowed()lifetime policy.get_next_sequential_gatesandget_combinational_conenow defaultforbidden_pinsto an empty set, matching the C++ side.dict[Net, float]returns inboolean_influence).Docs
Sphinx entries for the new enums/classes; the wiki (Decorators, Netlist-Utilities, Bitorder-Propagation pages) is already updated and pushed, including the
get_combinational_conerename.Verification
get_gates, both-is-union, cache-survives-cycles,TraversalCache, shortest-path-to-module)get_pathand forbidden-pin filtersTraversalCachepath 1522/1522 identical and ~2× faster from Pythonget_gateswithat_mismatchand the old name is gone🤖 Generated with Claude Code
Also on this branch
SMT.QueryConfig,SMT.Constraint,SMT.ModelandSMT.SolverResultnow print their content from Python (to_string+__str__; C++ gained the three missingto_stringmembers), and a printed model no longer starts with a stray comma. Off-topic for the traversal work, kept here at the author's request since Give a bit order a type, make the SMT interface usable, and let the call policy reach a property #642 is merged.netlist_traversal_decorator.cppnow includes<deque>itself — libstdc++ does not provide it transitively, which failed every Ubuntu job while macOS built clean.