-
Notifications
You must be signed in to change notification settings - Fork 934
Updating test_cell #5024
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
base: main
Are you sure you want to change the base?
Updating test_cell #5024
Conversation
@povik Can you update the handling for |
@KrystalDelusion please see this patch and #5025
In case you're wondering this shouldn't be removing any coverage on the |
Needs updating to `$macc_v2`.
Still unsupported: - wide muxes (`$_MUX16_` and friends) Partially supported types have comments in `test_cell.cc`. Fix `CellTypes::eval() for `$_NMUX_`. Fix `RTLIL::Cell::fixup_parameters()` for $concat, $bwmux and $bweqx.
If the cell type has a S signal and hasn't already been handled, use `CellTypes::eval(cell, A, B, S)`.
`-simlib` also doesn't work.
`CellTypes::eval()` is more generic but also more limited. `ConstEval::eval()` requires more setup (both in code and at runtime) but has more complete support.
c9c1337
to
66f66d5
Compare
Rebased on latest main to include #5025 |
Includes special cases for partially supported cells.
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.
We can see later if it is too costly to spawn new CI machine just for this, but guess additional useful tests could be added here, or we can merge it with running yosys-tests.
Looking at the time taken for the checks on this PR, the test_cell tests took 16m, while regular Ubuntu tests took just 15m. It is running 100 tests on each cell type though, so I think if we did move it into the main yosys tests we should reduce that that. It might also be a good idea to use a fixed seed for better repeatability, I think I was going to do that but forgot while waiting for the CI to run and then got distracted doing something else 😅 |
What are the reasons/motivation for this change?
The
test_cell
pass hasn't been kept fully up to date, and there are a number of cells marked asevaluable
but not supported by it (or in some cases, byConstEval::eval()
which it uses).$pmux
is also only supported when using the-edges
flag, though it's not specified as to why. This is spun out of #4910.$macc
support is also out of date since the change to$macc_v2
.Explain how this is achieved.
test_cell
with support for (most) of theevaluable
cells, gated bynosat
ortechmap_cmd.compare
as needed.ConstEval::eval()
base case to work with$bwmux
.Add wide mux testingspun out to Wide mux cells ($_MUX4_ et al) are incorrectly marked as evaluable #5035$macc
cell to use$macc_v2
.CellTypes::eval()
about usingConstEval
.test_cell
to CIIf applicable, please suggest to reviewers how they can test the change.
Call
test_cell all
.$pow
and$pmux
require-nosat
and-aigmap
flags.$eqx
,$nex
, and$bweqx
require-nosat
and either-aigmap
or-simlib
flags.$buf
requires either-aigmap
or-simlib
flags.OR
Check
test-cells
CI job ("Build and run tests/Run test_cell" in Checks tab)