Skip to content
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

Add splitfanout pass #4741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add splitfanout pass #4741

wants to merge 1 commit into from

Conversation

akashlevy
Copy link
Contributor

@akashlevy akashlevy commented Nov 14, 2024

What are the reasons/motivation for this change?

This PR adds a splitfanout pass that splits cells with fanout > 1 into copies, each with fanout 1. It is effectively the opposite of opt_merge and is useful for (1) enabling optimizations that cannot work when there is fanout, (2) splitting high fanout cells/nets that might cause problems in PD.

The pass can operate on selections and has a -limit flag to selectively operate on cells with fanout < specified limit. It does not operate on "bit-split" cells (word-level cells where different bits of the word have different/non-contiguous destinations), so splitcells should be run first if fanout splitting is desired for such cells.

Explain how this is achieved.

The pass works by building a database of drivers and loads, then operating in reverse topological order on selected cells. It checks that each cell is not "bit-split", then makes a copy to drive each load. The original cell is used to drive the final load.

  • passes/cmds/splitfanout.cc: splitfanout pass
  • passes/cmds/Makefile.inc: Add the new source

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

  • YosysHQ to review source code and provide feedback/edits as necessary
  • YosysHQ to construct test plan of 15-20 small-medium test cases
  • Silimate to review test plan and sign off
  • YosysHQ to write test cases according to test plan and add to regression
  • YosysHQ to provide formal equivalence checking scripts with equiv_opt or FOSS eqy
  • Silimate to sign off on test case implementation
  • YosysHQ to merge PR

Copy link
Collaborator

@widlarizer widlarizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this functionality is needed, but the implementation approach seems solid and it's not redundant with existing passes

Module *module;
SigMap sigmap;
dict<SigBit, tuple<IdString,IdString,int>> bit_drivers_db;
dict<SigBit, pool<tuple<IdString,IdString,int>>> bit_users_db;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction is copied from splitcells.cc, it would be best to leave a comment that marks it as such for the sake of a future rework touching both instances of this pattern. I think some rework is possible to reduce the usage of these indices and make them more readable, but for the most part, if the runtime and memory consumption of the current implementation of splitfanout is sufficient for your use case then there isn't any immediate need for that.

Some of the possible improvements:

  • In buffer-normalized mode (see bufnorm.cc), bit_drivers_db wouldn't be needed
  • The construction of toposort, construction of the indices, and final cell duplication could be done with fewer passes over the cells. Though this would also make the pass harder to modify, read, maintain

SplitfanoutWorker(Module *module) : module(module), sigmap(module)
{
// Add nodes to topological sorter for all selected cells
log("Making toposort nodes for module %s...\n", log_id(module));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, most of the log statements should be demoted to log_debug. For example, opt_clean only logs how many cells and wires it removed. Listing every cell that's ineligible for splitting is very verbose

int foi = 0;
cell->unsetPort(outport);
int num_new_cells = GetSize(bit_users)-1;
int bit_user_i = num_new_cells;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for counting down here?

}
void execute(std::vector<std::string> args, RTLIL::Design *design) override
{
int limit = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider std::optional<int>

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