formal: add SymbiYosys proof for cc_ecc_encode/cc_ecc_decode - #355
Merged
Conversation
Prove the four cases documented in the cc_ecc_decode header for the encoder/decoder pair: clean round-trip, single-bit correction inside the Hamming codeword, a flipped extended parity bit, and double error detection. One encoder feeds four decoders so the cases cannot interfere. DataWidth 1, 2, 4, 5, 11 and 12 are proved by default, bracketing each parity-width transition on both sides. Wider parametrizations up to the module default of 64 run as a separate sweep task, not part of `make all`. The properties claim nothing about data_o under a double error, nothing beyond two flipped bits, and nothing about interoperability with another implementation of the same code. The parity width is checked against a count of the power-of-two codeword positions rather than by recomputing the cc_pkg search, so a defect there is not reproduced by the check. `make ecc.check` runs a cover task alongside the proof, so a proof resting on an unsatisfiable assumption cannot pass unnoticed.
Collaborator
|
Coverage for |
cc_ecc_decode exposes syndrome_o, but the output was never driven. Drive it from the decoder's internal syndrome and add formal coverage for the documented syndrome behaviour: - clean codeword produces zero syndrome - a single codeword-bit error produces its 1-based position - an extended-parity-only error produces zero syndrome - two distinct codeword-bit errors produce nonzero syndrome The double-error property deliberately does not constrain the exact syndrome value because the decoder interface does not promise one.
Contributor
Author
|
Thanks — While wiring this up I found that |
Collaborator
|
Looks all good, also cool that we found a real bug, more verification is doing something! |
Contributor
Author
|
Very cool indeed! 🙌 |
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.
Adds a SymbiYosys proof for the
cc_ecc_encode/cc_ecc_decodepair, as a subtask of #288.Following your note on #288, this does not wait on #354. I used #354 only as a rough reference and kept the layout to what current
masteralready uses forlzcandheaviside, so it should rebase cleanly onto whatever #354 settles on.cc_ecc_decodesuits formal work because its header already carries the specification. The syndrome and parity truth table gives four cases over a combinational pair with no protocol and no state, so the properties are a transcription of documented behaviour rather than a new model.What is proved
One encoder feeds four decoders, each seeing a different corruption of the encoded word, so no case can weaken another.
data_o == data_i, all three flags lowdata_o == data_i,single_error_ohigh, other flags lowdata_o == data_i,parity_error_ohigh, other flags lowdouble_error_ohigh, other flags lowP2 excludes the extended parity bit deliberately, since a flip there is reported through
parity_error_oand stating P2 over the whole encoded word would contradict the truth table.What is not claimed
data_ounder a double error. The decoder does not promise a value there, and the syndrome can address a position outside the codeword. That decoder instance has itsdata_oleft unconnected so no property can read it.One check is not matched-pair.
cc_ecc_properties.svcounts the power-of-two positions in the codeword and compares that count againstcc_pkg::ecc_get_parity_width, rather than recomputing the same search, which would agree with the package for the same wrong reason. Both operands are elaboration-time constants, so it is an elaboration check and not a proof obligation, and the comment says so.Widths and engine
make ecc.checkprovesDataWidth1, 2, 4, 5, 11 and 12.cc_pkg::ecc_get_parity_widthadds a parity bit going from 1 to 2, from 4 to 5 and from 11 to 12, so each transition is proved on both sides. 1, 4 and 11 are also the widths whose encoded word is exactly a power of two bits wide, where an index constraint is easiest to get wrong.Engine is
abc pdrrather than thesmtbmcthe other proofs use. The pair is purely combinational, so depth 1 is already exhaustive, and PDR is chosen only for runtime: on the default width set the proof takes about 10 s with PDR against about 3 min with smtbmc. I did not run smtbmc to completion on the full sweep, so I am not putting a number on that.Wider parametrizations up to the module default of 64 are a separate task, not part of
make all. It proved all 128 properties in 46 min 47 s here:Vacuity
make ecc.checkruns acovertask alongside the proof. The properties constrain where the injected errors may land, so the cover task is what shows those positions are reachable;abc pdrreports no PREUNSAT, so a proof resting on an unsatisfiable assumption would otherwise pass silently.That is not hypothetical. An earlier version of this harness shared the error-position inputs across widths, which let one width's constraint narrow another's reachable positions while the proof stayed green. The cover task is what caught it, which is why it runs by default rather than on request.