write_blif: name CIs after the node, not the topological index - #704
Open
marcelwa wants to merge 1 commit into
Open
write_blif: name CIs after the node, not the topological index#704marcelwa wants to merge 1 commit into
marcelwa wants to merge 1 commit into
Conversation
`topo_view` reimplements `node_to_index` as the position in the topological order,
while every other reference in this writer -- fanin lists and PO bridges -- names a
node by its raw id. The two agree only while the CI node ids happen to be
contiguous.
They are not contiguous in general. A `klut_network` produced by `lut_map` has gaps,
and so does any network where a primary input is created after a gate. There the
`.inputs` line declares names nothing reads, and the `.names` bodies reference names
that were never declared:
.inputs pi2 pi3 pi4
...
.names new_n4 pi5 new_n6
`pi4` is dead and `pi5` is undeclared. Lorina's own BLIF reader rejects that, but ABC
accepts it and ties the undeclared signal to constant 0 -- so the netlist reads back
as a well-formed circuit computing something else, with no error anywhere.
Found by combinational equivalence checking a mapped EPFL `mem_ctrl`, which came back
NOT_EQUIVALENT with 39 phantom inputs.
Fixed by deriving the name from the node, which is what the rest of the writer does.
The added test builds the smallest network with a CI gap and pins the exact output.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #704 +/- ##
==========================================
- Coverage 84.07% 84.06% -0.02%
==========================================
Files 190 190
Lines 29513 29513
==========================================
- Hits 24813 24810 -3
- Misses 4700 4703 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
write_blifnames combinational inputspi{topo_ntk.node_to_index(n)}, while every other reference in the same writer — fanin lists and the PO bridges — names a node by its raw id.topo_viewreimplementsnode_to_indexas the position in the topological order, so the two agree only for as long as the CI node ids happen to be contiguous.They are not contiguous in general. A
klut_networkproduced bylut_maphas gaps in them, and so does any network where a primary input is created after a gate.What it produces
The smallest case — two PIs, an AND, then a third PI:
Why it is worth fixing rather than working around
The output is not malformed in a way anything reports.
lorina::read_blifdoes reject it, but ABC accepts it and ties the undeclared signal to constant 0 — so the file reads back as a well-formed circuit that computes a different function, silently, with the right port count.I hit this running combinational equivalence checking over LUT-mapped EPFL circuits:
mem_ctrlcame back NOT_EQUIVALENT with 39 phantom inputs, and it took a while to believe the writer rather than the mapper.The change
Two lines: derive the name from the node, which is what the rest of the writer already does. The named-network branch has the same problem — it calls
make_signal(node_to_index(n)), sohas_name/get_nameare looked up against the wrong signal — and is fixed the same way.The added test builds the smallest network with a CI gap and pins the exact output. It fails on master (both the string comparison and the
blif_read_after_write_teston top of it) and passes with the change; the rest of the suite is unaffected — 917 test cases, 133227 assertions green.