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

Automatically call hierarchy if the current pass does not accept abstract modules #4089

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

Conversation

whitequark
Copy link
Member

@whitequark whitequark commented Dec 21, 2023

Literally nobody who has ever used Yosys, in the entire history of Yosys, has wanted to see a $abstract\foo anywhere in the netlist. I have been encountering this issue for several years now. Enough.

@nakengelhardt
Copy link
Member

I disagree with this change, it would make it impossible to load certain files as command line arguments, so you couldn't write scripts that work on any valid input without knowing the file name any more.

There are many passes that do not work correctly until you run hierarchy -top foo, just like there are many passes that don't work until you run proc. Introducing more workarounds that make it sometimes ok to skip at the expense of breaking certain inputs doesn't improve the situation IMO, the solution here is to make it clear that users should always run hierarchy and proc immediately unless they know why they are not doing it. (The long-term solution is to make yosys aware of the different design states and the guarantees associated with them, and we have plans in this regard, hopefully there will be some movement on those next year.)

@whitequark
Copy link
Member Author

whitequark commented Dec 21, 2023

There are many passes that do not work correctly until you run hierarchy -top foo, just like there are many passes that don't work until you run proc.

Are there any passes that work correctly while there are $abstract modules in the design? (Aside from hierarchy itself, obviously.)

The more general problem here is that the change that delayed module instantiation until hierarchy has seriously regressed Yosys CLI's usability with Verilog inputs, including breaking existing scripts and invocations that did not have hierarchy. Do you not care about that?

If this change is rejected I will probably look into updating the Verilog frontend to not create $abstract modules if there are no parameters in the AST. I think the current situation is unacceptable as the impact on the CLI's usability is too high.

What makes the current situation worse is that unlike with proc, where passes that cannot operate on processes print a message indicating they fail, passes that cannot operate on $abstract modules silently do the wrong thing. They should at least print a message. If I wasn't too sick to work at the time this was introduced I would have strongly opposed it at the time. I could not, so I will strongly oppose this now.

@whitequark whitequark changed the title Automatically call hierarchy after reading command line inputs Automatically call hierarchy if the current pass does not accept abstract modules Dec 21, 2023
@whitequark
Copy link
Member Author

whitequark commented Dec 21, 2023

I disagree with this change, it would make it impossible to load certain files as command line arguments, so you couldn't write scripts that work on any valid input without knowing the file name any more.

Having thought about the reasoning here I agree that the change I proposed is wrong.

The long-term solution is to make yosys aware of the different design states and the guarantees associated with them, and we have plans in this regard, hopefully there will be some movement on those next year.

I would be disappointed to have to wait a year or more for something so basic. This pull request now implements awareness of the design state where there are abstract modules in the design, and automatically runs hierarchy in case the pass does not support it.

In principle the same could be done for proc but I have not made that change for two reasons:

  • The requirement to run proc has been in Yosys since the first release, so it had not broken anyone's scripts or muscle memory.
  • There is existing functionality to handle the existence of processes, and the change would be a lot more invasive, with a much smaller benefit.

I do think that something similar should be happening for processes (and write_cxxrtl does work that way) but I also think the tradeoff is not right for introducing it in this PR.

@whitequark whitequark force-pushed the auto-hierarchy branch 6 times, most recently from 73342a0 to d5aac9a Compare December 21, 2023 23:55
@whitequark
Copy link
Member Author

Are there any passes that work correctly while there are $abstract modules in the design?

(This got an answer in form of the abstract_modules_ok = true; statements added in the final patch.)

Whenever there are abstract modules in the netlist and the pass does not
declare that it supports them, call `hierarchy -auto-top` or `hierarchy`
(depending on whether there is a module marked `top`) to instantiate
them first. This makes several CLI workflows much more usable, including
one as basic as:

    $ yosys file.v -o file.il
@whitequark
Copy link
Member Author

We discussed this on today's Dev JF. We decided that the classification of passes into pre-hierarchy and post-hierarchy that is done in this PR should stay, that RTLIL::Design should gain a "stage" enum that indicates whether the design is pre- or post-hierarchy, and that running a post-hierarchy module on a pre-hierarchy design should unconditionally run the hierarchy pass.

We have also concluded that it may be useful to extend this to cover pre- and post-proc passes.

@whitequark whitequark marked this pull request as draft January 16, 2024 12:19
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