Skip to content

Conversation

KrystalDelusion
Copy link
Member

@KrystalDelusion KrystalDelusion commented Feb 21, 2025

What are the reasons/motivation for this change?

#4122 and related. Also c++ code can use the RTLIL::builtin_ff_cell_types(), but there is no corresponding method for selecting builtin ff cell types from yosys scripts.

Explain how this is achieved.

  1. Add (and provide values for) more CellType properties than just is_evaluable.
  2. Allow these properties to be matched with the select command.
    a. Enables e.g. calling delete y:synthesizable y:formal %u %n to delete all cells that are not synthesizable or formal

Open questions:

  • Should RTLIL::builtin_ff_cell_types() be removed and RTLIL::builtin_ff_cell_types().count(cell->type) be replaced with yosys_celltypes.get_cell(cell->type)->is_builtin_ff? Reduces redundancy, but requires adding #include "kernel/celltypes.h" in many of the files.
  • Should is_builtin_ff be renamed to is_sequential or something, with builtin ffs being defined as is_internal and is_sequential?
  • Should RTLIL::Cell::has_memid() and RTLIL::Cell::is_mem_cell() be replaced with cell type properties?

If applicable, please suggest to reviewers how they can test the change.

Call yosys -h "-celltypes" to check the assigned cell type properties make sense.

Add `CellTypes::setup_{comb, ff, formal}_type()`.  Most of the existing internal cells fit into one of these three groups.
Add `CellTypes::get_cell()` which takes a 'type' `IdString` and returns the corresponding `CellType` if it is in the `cell_types` dict, otherwise returns `nullptr`.
Useful for getting a cell type by name and then checking its attributes.
Looks up the cell type for each cell in the design, returning the value of `CellType::is_<property>`.
Only works for exact match, and only for internal cells.

Also add a simple test checking a small design with an $add cell and an $sdff cell.
@KrystalDelusion KrystalDelusion added the discuss needs further discussion on the YosysHQ discourse (https://yosyshq.discourse.group) label Feb 21, 2025
Now with `is_internal`, `is_metainfo`, and `has_effects`.
Each type property has a comment specifying usage/meaning.
Type properties (sans `is_internal`) get a default value of false.
`CellTypes::setup_*_type()` methods get their own `CellType` struct assignments with named fields being assigned true to improve readability.
celltypes: Extra type props

Now with `is_internal`, `is_metainfo`, and `has_effects`.
Each type property has a comment specifying usage/meaning.
Type properties (sans `is_internal`) get a default value of false.
`CellTypes::setup_*_type()` methods get their own `CellType` struct assignments with named fields being assigned true to improve readability.

Also remove `is_evaluable` from cell types that aren't actually `eval`-able,
instead using `is_combinatorial` which afaict is what the flag was being used as a proxy for.
@KrystalDelusion KrystalDelusion removed the discuss needs further discussion on the YosysHQ discourse (https://yosyshq.discourse.group) label Feb 28, 2025
Like `help -cells` but instead of printing the cell's ports, prints its flags.
Turns out using designated initializers is c++20, so not available while we are still c++17 friendly.
@KrystalDelusion KrystalDelusion marked this pull request as ready for review March 8, 2025 01:58
@KrystalDelusion KrystalDelusion added the status-needs-review Status: Needs reviewers to move forward label Mar 10, 2025
@KrystalDelusion KrystalDelusion marked this pull request as draft March 29, 2025 05:02
@KrystalDelusion KrystalDelusion removed the status-needs-review Status: Needs reviewers to move forward label Mar 29, 2025
@KrystalDelusion
Copy link
Member Author

So that I don't lose them, this touches on #3913, #4638, and #4034.

Currently working on updating passes/tests/test_cell.cc to also check CellTypes::eval(), but it seems like a non-zero number of 'evaluable' cells, which don't have a CellTypes::eval() implementation, do have a ConstEval::eval() implementation. Not all ($pow and $concat for instance are missing), but definitely some (like $fa and $alu). Some have duplicate definitions, such as many of the $mux family of cells; and for double fun $bmux makes a call to RTLIL::const_bmux in both versions, but has some extra checks that are only in the ConstEval version. And then the ConstEval base case is to just call CellTypes::eval.

Where possible at least, and disable-able with `-nocteval`.
`$lcu` doesn't work because it uses ports `P, G, CI, CO` instead of the standard `A, B, C, Y`.
`$alu` and `$fa` have more than one output so they don't work either (and in the case of `$alu` it has extra inputs too).
`$macc` is at least supported, but `CellTypes::eval()` doesn't have an implementation so it fails (which would also be true for `$lcu`, `$alu`, and `$fa`; if they weren't being rejected based on ports).

Also add `test_cell -list [all|evaluable|missing]` which prints the list of cell types supported by test_cell, cell types marked evaluable, and cell types marked evaluable but not supported by test_cell respectively.  Potential for listing cell types supported by test_cell but *not* marked evalulable, though that list is currently empty.

Add `tests/various/evaluable.sh` to exercise this.
@KrystalDelusion
Copy link
Member Author

KrystalDelusion commented Mar 30, 2025

CellTypes::eval() missing support for test_cell cell types:
$macc
$lcu
$alu
$fa

ConstEval::eval() missing support for test_cell cell types:
$bwmux

test_cell missing support for evaluable cell types:
$_MUX16_
$_MUX8_
$_MUX4_
$pmux
$bweqx
$nex
$eqx

+ partial support for $buf and $pow.

The $pmux there is interesting because it is handled in both CellTypes and ConstEval, but test_cell only includes it when doing -edges which doesn't do the normal testing, and trying to include it doesn't seem to work.

$eqx and $nex have a custom ConstEval::eval() implementation, which no longer matches the behaviour since x-prop behaviour was redefined, and $bweqx doesn't work in sat or ConstEval::eval().

Which I think puts the list for cells marked evaluable but not eval-able (for some definition of eval) as:

$macc
$lcu
$alu
$fa
$_MUX16_
$_MUX8_
$_MUX4_

Unless you use ConstEval::eval(), in which case it's:

$_MUX16_
$_MUX8_
$_MUX4_

Oh, and $macc has its own Yosys::Macc::eval(), just for fun.

EDIT: Updated the supported cells.

Still unsupported:
- x-prop cells ($eqx, $nex, $bweqx)
- wide muxes (`$_MUX16_` and friends)
- $pmux

Partially supported:
- $bwmux is not supported in `ConstEval::eval()`, works with `-noeval`
- $buf has no mapping in techmap.v so is unusable with `techmap -assert` (i.e. the default)
- $pow has `_TECHMAP_FAIL_` in techmap.v, `-simlib` works for some iterations but fails for others, `-aigmap` works fine

Fix `CellTypes::eval() for `$_NMUX_`.
Fix `RTLIL::Cell::fixup_parameters()` for $concat and $bwmux.
It fails in sat, and in `ConstEval::eval()`.  But it's pseudo supported in test_cell at least.

Also fix `RTLIL::Cell::fixup_parameters()` for $bweqx.
@KrystalDelusion KrystalDelusion mentioned this pull request Apr 15, 2025
7 tasks
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.

2 participants