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

Merge of function black boxing #4588

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

Merge of function black boxing #4588

wants to merge 16 commits into from

Conversation

kendroe
Copy link

@kendroe kendroe commented Sep 8, 2024

What are the reasons/motivation for this change?

I added a new feature to allow Verilog functions to be black boxed. In the generated smt2 file, an euf symbol is used instead
of expanding the function definition. Check out my Linkedin blog here https://www.linkedin.com/feed/update/urn:li:activity:7191433053066448896/. This blog links to earlier blogs describing the changes.

Explain how this is achieved.

Many changes to code generation.

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

I expect to do a lot of cleanup before these changes are accepted.

There are lots of debugging prints that should be removed. It would be good for someone with lots of experience with the internals of Yosys to review.

@kendroe kendroe requested a review from zachjs as a code owner September 8, 2024 15:30
Copy link
Member

@povik povik left a comment

Choose a reason for hiding this comment

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

Indeed there a lot of left-over debug prints, but let's focus on the fundamental approach first. I have only skimmed the changes, but is the basic idea that function calls are deferred, that is they are converted to $fun cells in the frontend, and the user has the option to either blackbox those or replace them with the function's body later on?

@@ -263,7 +265,7 @@ namespace AST

// simplify() creates a simpler AST by unrolling for-loops, expanding generate blocks, etc.
// it also sets the id2ast pointers so that identifier lookups are fast in genRTLIL()
bool simplify(bool const_fold, int stage, int width_hint, bool sign_hint);
bool simplify(bool const_fold, int stage, int width_hint, bool sign_hint, bool in_param, bool aux_modules = false);
Copy link
Member

Choose a reason for hiding this comment

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

This seems to partially revert #3813 and is introducing a new flag, which we want to avoid if it can be helped


RTLIL::Module *module;
RTLIL::IdString name;
AST::AstNode *def;
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell this new Function object is here so that the ASTs of functions can be looked up. What is the AST used for?

This is adding a new type of member to modules (next to wires, cells, processes and memories). This means these now have to be considered all over the Yosys codebase anywhere we are manipulating modules and they should be saved/restored on writing out an RTLIL file.

That's a substantial addition in complexity to serve one feature. Preferably we encode what we need in parameters of the new cell type $fun instead (if we need a new cell type in the first place).

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