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

[major] Finalize FIRRTL 4.0.0 Public Modules #167

Merged
merged 5 commits into from
Feb 9, 2024

Conversation

seldridge
Copy link
Member

Remove some of the changes for public modules to make these easier to implement without dramatic changes to how Chisel is invoked and requiring roll out of a lot of changes in FIRRTL compilers. Specifically, continue to keep the "main module" and thereby require that a FIRRTL circuit has a single entry point.

The FIRRTL ABI is mostly unmodified with these changes.

The intent is to remove the circuit completely in FIRRTL 5.0 and delay changes to Chisel and FIRRTL compilers until then. (There's a lot in FIRRTL 4.0 already and it would be good to cut it somewhere.)

Remove some of the changes for public modules to make these easier to
implement without dramatic changes to how Chisel is invoked and requiring
roll out of a lot of changes in FIRRTL compilers.  Specifically, continue
to keep the "main module" and thereby require that a FIRRTL circuit has a
single entry point.

The FIRRTL ABI is mostly unmodified with these changes.

The intent is to remove the circuit completely in FIRRTL 5.0 and delay
changes to Chisel and FIRRTL compilers until then.  (There's a lot in
FIRRTL 4.0 already and it would be good to cut it somewhere.)

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
spec.md Outdated
Comment on lines 119 to 121
A circuit that contains no public modules is trivially equivalent to an empty circuit.
It is not enforced that a circuit has at least one public module.
It is not enforced that the main module is public.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines seem strange to me.

How can a circuit be empty if:

  • it requires a name, and
  • the name matches a public module

It feels like we're trying to document a quirk of the implementation. Maybe it would help to comment on the intention of these rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe there's a better way of putting this...

The PR is trying to make public mean the same thing for all modules. If a module is public then you get a Verilog module with ports following the port lowering ABI. If a module is private, anything can happen to the module. It may be optimized away, it may get inlined, it may be duplicated a million times, etc.

The main module creates a discontinuity arising from making the main module implicitly public. If it is implicitly public, then either the public keyword has no effect or the public keyword is mandatory (or the public keyword cannot be applied to the main module, but it is implicitly public anyway).

The language in this section is trying to say that a circuit which has no public modules can be optimized to produce no Verilog.

WDYT?

For full clarity, this is an intermediate step to FIRRTL 5. Currently you can get the "bag o' dags" representation in FIRRTL 4 with:

circuit Container :
  public module Foo :
  public module Bar :
  module Container :
    inst foo of Foo
    invalidate foo

    inst bar of Bar
    invalidate Bar

or perhaps the following, though FIRRTL compilers don't really handle this well right now:

circuit Dummy :
  public module Foo :
  public module Bar :
  module Dummy :

In FIRRTL 5 we can drop the circuit entirely and just make this:

public module Foo:
public module Bar:

@seldridge seldridge merged commit f385223 into main Feb 9, 2024
1 check passed
@seldridge seldridge deleted the dev/seldridge/simple-public-modules branch February 9, 2024 18:09
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