Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Updating test_cell #5024

wants to merge 8 commits into from

Conversation

KrystalDelusion
Copy link
Member

@KrystalDelusion KrystalDelusion commented Apr 15, 2025

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 as evaluable but not supported by it (or in some cases, by ConstEval::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.

  • Update test_cell with support for (most) of the evaluable cells, gated by nosat or techmap_cmd.compare as needed.
  • Fix ConstEval::eval() base case to work with $bwmux.
  • Add wide mux testing spun out to Wide mux cells ($_MUX4_ et al) are incorrectly marked as evaluable #5035
  • Update $macc cell to use $macc_v2.
  • Add comments for partially supported cell types.
  • Add comment to CellTypes::eval() about using ConstEval.
  • Add test_cell to CI

If 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)

@KrystalDelusion
Copy link
Member Author

@povik Can you update the handling for $macc_v2 in the create_gold_module function in test_cell.cc? Just changing it from $macc to $macc_v2 fails the internal cell checker. And leaving it as $macc doesn't work because it uses Yosys::Macc::to_cell() which converts it to a v2 and then fails the internal cell checker.

@povik
Copy link
Member

povik commented Apr 15, 2025

@KrystalDelusion please see this patch and #5025

diff --git a/passes/tests/test_cell.cc b/passes/tests/test_cell.cc
index 39b7ccd3a..f42255803 100644
--- a/passes/tests/test_cell.cc
+++ b/passes/tests/test_cell.cc
@@ -167,7 +167,7 @@ static RTLIL::Cell* create_gold_module(RTLIL::Design *design, RTLIL::IdString ce
                cell->setPort(ID::CO, wire);
        }
 
-       if (cell_type == ID($macc))
+       if (cell_type == ID($macc_v2))
        {
                Macc macc;
                int width = 1 + xorshift32(8 * bloat_factor);
@@ -201,6 +201,7 @@ static RTLIL::Cell* create_gold_module(RTLIL::Design *design, RTLIL::IdString ce
                        this_port.do_subtract = xorshift32(2) == 1;
                        macc.ports.push_back(this_port);
                }
+
                // Macc::to_cell sets the input ports
                macc.to_cell(cell);
 
@@ -208,12 +209,6 @@ static RTLIL::Cell* create_gold_module(RTLIL::Design *design, RTLIL::IdString ce
                wire->width = width;
                wire->port_output = true;
                cell->setPort(ID::Y, wire);
-
-               // override the B input (macc helpers always sets an empty vector)
-               wire = module->addWire(ID::B);
-               wire->width = xorshift32(mulbits_a ? xorshift32(4)+1 : xorshift32(16)+1);
-               wire->port_input = true;
-               cell->setPort(ID::B, wire);
        }
 
        if (cell_type == ID($lut))
@@ -937,7 +932,7 @@ struct TestCellPass : public Pass {
                cell_types[ID($sop)] = "*";
                cell_types[ID($alu)] = "ABSY";
                cell_types[ID($lcu)] = "*";
-               cell_types[ID($macc)] = "*";
+               cell_types[ID($macc_v2)] = "*";
                cell_types[ID($fa)] = "*";
 
                for (; argidx < GetSize(args); argidx++)

In case you're wondering this shouldn't be removing any coverage on the B port as all A, B, C ports get populated by the helper for macc_v2.

KrystalDelusion and others added 7 commits April 22, 2025 10:05
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.
@KrystalDelusion
Copy link
Member Author

Rebased on latest main to include #5025

Includes special cases for partially supported cells.
@KrystalDelusion KrystalDelusion requested a review from mmicko as a code owner April 21, 2025 22:20
Copy link
Member

@mmicko mmicko left a 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.

@KrystalDelusion
Copy link
Member Author

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 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants