-
Notifications
You must be signed in to change notification settings - Fork 678
Introduce an OSS Formal Flow #2309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
marnovandermaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to split the formal fixing changes from the open source flow (as you suggested in person). Here are just a few comments based on an initial read though. Todo for me is read through conductor.py and aig-manip
32dd8e2 to
ff5bcef
Compare
780fde9 to
fad268c
Compare
marnovandermaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need to run a clang format and a Verible lint to make CI happy.
marnovandermaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewing conductor.py
Really cool stuff and some comments for you to address while we get more feedback.
machshev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Some nits...
| step = int(step[4:]) | ||
|
|
||
| if name.endswith("_Cover") or step > args.step or (step == args.step and len(args.props) > 0 and name not in args.props): | ||
| print(f"Delete {name} ({id})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally preferable to use log.info instead of print for massages you always want to see. Alternatively log.debug for messages that should only show in verbose mode.
34d04aa to
5c4f560
Compare
marnovandermaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. I still need to run the flow again before I approve.
marnovandermaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proof ran through to me so I am happy to approve it. I would like at least one other person to approve this before we merge, this can be @nbdd0121 @hcallahan-lowrisc @machshev or @jwnrt .
The only thing that I can currently think of that would be good is to change the maximum memory to half of the total in Pomona.
jwnrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Briefly reviewed the Rust and didn't see any problems, LGTM
5c4f560 to
7c9b433
Compare
hcallahan-lowrisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I'm super happy to see this get merged. Well done @mndstrmr for all the great work!
One note before merging. Some files seem to be missing copyright/license headers. @marnovandermaas would you mind going through all the formal files and checking?
|
|
||
| # The aiger file for the OSS flow, using the yosys_formal/* plugins. The main yosys script is in build_all.ys | ||
| build/all.aig: build/psgen-yosys.sv build/fusesoc build/ibexspec.sv check build_all.ys $(LOWRISC_YOSYS_SLANG) build/yosys_formal.so | ||
| yosys -m $(LOWRISC_YOSYS_SLANG) -m build/yosys_formal.so -v 1 -s <(cat ./build_all.ys | sed "s|LOWRISC_SAIL_SRC|$(LOWRISC_SAIL_SRC)|g") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly annoying yosys can't do environment variable substitution, would be nice to not have to do this dance with sed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It unfortunate. Even using envsubst won't work due to this line:
Line 18 in eb6e0ac
| delete */t:$print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to turn build_all.ys into a Tcl script and look up the environment via $::env(FOO). Yosys has an embedded Tcl interpreter in most builds
5427262 to
eb6e0ac
Compare
It's nice to see that halving the memory does in fact double the runtime, as expected. 200GB does 43 minutes, 100GB did 81 minutes. See this run. |
|
Nice, 80m is still within my personal comfortable range for a PR workflow given that it's not blocking or in the merge queue. |
eb6e0ac to
14aef04
Compare
|
That should be the final force push (a missing license header in Cargo.toml) done now. |
buildspec.py previously allowed us to cut out specific instructions from
the specification, this allowed for faster build times and so a tighter
feedback loop.
Now, building and elaborating the Sail specification is fast enough that
this is no longer necessary.
We can therefore remove `ISS_instr and {ex,wbexc}_is_pres_instr, as
weel as instrs.json.
Also renames Sources.mk to sail-sources.mk for clarity.
Previously we used -fun2wires 2:rX so that we could track which registers were being used. This was leading to combinational loops, and so had already been removed from CHERIoT-Ibex. We can still 'track' them using Ibex's own interpretation of which registers are relevant, and marking the rest as x.
Yosys-slang is a SystemVerilog frontend to yosys, which will be used in upcoming formal work. It takes issue with some parts of the formal flow. This commit cleans things up so that it may consume the RTL happily.
This commit introduces a formal flow for Ibex that does not depend on Jasper, instead using yosys and our fork of the yosys-slang frontend to produce an aiger file, which the rIC3 model checker, under the instruction of conductor.py, can then verify. Just one aiger file is created, and is subsequently reconfigured by the Rust aig-manip tool. See dv/formal/README.md for more details, but in general: 1. We generate the specification module and psgen RTL in roughly the same way as the Jasper flow, with small changes to the psgen output in particular. 2. We use a fork of yosys-slang as the frontend for yosys. It has comprehensive SystemVerilog support, and handles both the specification and Ibex easily. It lacks SVA support by default, hence the need for the fork, though hopefully this will be upstreamed soon. The RTL source list is constructed by fusesoc in the same way as for Jasper. 3. The yosys build script is then specified in build_all.ys. It makes use of three custom passes, which may be found in yosys_formal. See their source files for more explanation of what they do. Yosys takes ~10 minutes to build an aiger file. See aig-manip/src/main.rs for more description of aiger files. 4. In order to prove a property, the rIC3 model checker can be used. However, the aiger file produced by yosys in step 3 contains all properties as assertions. To focus on just one, and to take all prior steps as assumptions the aig-manip tool is used. After this invocation of rIC3 is straightforward for IC3, k-induction or BMC. If a CEX is discovered it may be analysed by the aig-manip tool, which can lift the AIW (aiger witness) into a VCD file, for use in gtkwave etc. 5. When considering the proof as a whole, the conductor.py script is used to discover proof strategies, and then run proofs while maximising memory usage. It caches proofs for each step under strategies/step*.json. Additonal notes: 1. The actual proof is relatively unchanged, with just a couple of new properties. 2. The proof is fast (it takes 40 minutes or so on a good machine) 3. smt2 based proofs are also available, e.g. for k-induction, since the build script (build_all.ys) will produce an smt2 file, which can be manipulated by smt2manip.py in the same that aig-manip does for an aiger file. 4. The transformation made to a global clock will confuse clock gating. It *should* be fine, since the clock signal it generates is essentially ignored anyway. 5. This commit also switches from poetry to UV. 6. This runs on a small memory bound (the smallest possible bound). See check/protocol/mem.sv
This commit introduces a new ci job which invokes the OS formal flow. It uses the nix flake, which contains all the necessary dependencies. With everything already in cache this takes less than an hour on pomona. A parameter (MAX_MEM_GB) can be used to limit memory usage.
14aef04 to
4293c3c
Compare
This PR introduces a formal flow for Ibex that does not depend on Jasper, instead using yosys and our fork of the yosys-slang frontend to produce an aiger file, which the rIC3 model checker, under the instruction of conductor.py, can then verify. Just one aiger file is created, and is subsequently reconfigured by the Rust aig-manip tool.
This commit also introduces a new ci job which invokes the OS formal flow. It uses the nix flake, which contains all the necessary dependencies.
See dv/formal/README.md for more details, but in general:
Additonal notes: