[major] Add public modules

[seldridge]

Add a notion of "public" modules to FIRRTL. A public module is a module that has a defined Verilog ABI. A user can look at a FIRRTL circuit and expect to see Verilog modules with expected ports and names after compilation. The notion of a circuit is then expanded to allow for any number of public modules. The instance graphs of public modules may or may not share common children. In effect, a circuit is now collection of multi-rooted instantiation trees.

Any public module that is instantiated by other modules has certain restrictions on its lowering. It follows one of the port lowering ABIs and no information from its instantiating context(s) may be used to optimize into it. (You cannot constant prop into a public module.) However, it is allowable for modules which instantiate the public module to optimize through the public module.

Modify the ABI to require that all public modules produce a filelist whose name is derived from the circuit and public module.

Closes #82.
Closes #20.

[dtzSiFive]

cc #82 .

[mwachs5]

The notion of a circuit is then expanded to allow for multiple roots and these roots may have no common children.

What is the point of this proposal, if they can have no common children?

EDIT: This proposal eats, shoots, and leaves... i get it now that the roots are allowed to have no common children.

[seldridge]

It is a point of discussion if public modules should be allowed to be instantiated. This could work one of several ways if allowed:

1. A public, instantiated module is compiled as if it were an external module. I.e., all ports are marked don't touch and the same restrictions apply on uninferred widths and abstract reset.
2. A public, instantiated module is compiled with all optimizations allowed, but the ports have to follow one of the port lowering ABIs. This implies no uninferred widths and no abstract reset.
3. A public, instantiated module is compiled will all optimizations on and you get a Verilog file out, but you have no idea what the ports are until you compile it. 😅

[mwachs5]

because of this point 1, i feel like instead of calling this "public", 'root' would be a better word. That captures more the meaning. We may decide that public is applicable to things that do not meet this requirement 1, later.

[seldridge]

We can just remove this requirement. I think Will, you, and others have all convinced me that this isn't useful to enforce. I'm planning to back this out and then add language around the types of optimizations that are allowed here. Specifically, a public module that is instantiated must be compiled as if it were not instantiated. However, a module that instantiates a public module is allowed to optimize through a public module.

E.g., it is legal for a parent to inline a public module that it instantiates. However, that public module must still be produced following the Verilog ABI.

Initial circuit:

circuit Foo:
  public module Foo:
    inst bar of Bar

  public module Bar:
    wire w: UInt<1>

Final legal circuit:

circuit Foo:
  public module Foo:
    wire bar_w: UInt<1>
  public module Bar:
    wire w: UInt<1>

E.g., if a parent instantiates a public module and drives a port to a constant that is fed out through an output port, then the parent module can constant prop through the instance. However, the constant cannot be moved into the instance.

Initial circuit:

circuit Foo:
  public module Foo:
    output b: UInt<1>
    inst bar of bar
    connect bar.a, UInt<1>(1)
    connect b, bar.b
  public module Bar:
    input a: UInt<1>
    output b: UInt<1>
    connect b, a

Optimized circuit:

circuit Foo:
  public module Foo:
    output b: UInt<1>
    connect b, UInt<1>(1)
  public module Bar:
    input a: UInt<1>
    output b: UInt<1>
    connect b, a

Illegally optimized circuit:

circuit Foo:
  public module Foo:
    output b: UInt<1>
    connect b, UInt<1>(1)
  public module Bar:

[mwachs5]

Ah, if you don't enforce it then all @dtzSiFive questions I said didn't matter start mattering again ;) But i think that the rule that a public module has to exist in a verilog file of that name squashes many of the questions.

[mwachs5]

Specifically, a public module that is instantiated must be compiled as if it were not instantiated. However, a module that instantiates a public module is allowed to optimize through a public module.

Putting this in my own words:

• Optimizations on a module A that instantiates a public module B MAY treat B as if it were equivalent to an extmodule, but optimizations to A MAY also fully consider the contents of B and perform any modifications to A desired (including not instantiating B at all).

• When a module A instantiates a public module B, optimizations on B SHALL ignore any information known about its context in A

[mwachs5]

i am a little bit worried about the first bullet, sicne it might imply that you are actually essentially duplicating the public module into a "not public" copy and using that, which might not get the desired effect.

[mwachs5]

in the future we could add a thing at the instantiation site that is inst foo of Foo as public which means "use the public version of this and I mean it" to block the optimizations the other way.

[dtzSiFive]

Spec currently contains this language:

Modules have the property that instances can always be inlined into the parent
module without affecting the semantics of the circuit.

Inlining I think usually means replacing a thing with its definition, not necessarily deleting the original. Often the latter is kinda assumed since often after inlining it everywhere the definition is now dead. But as-is I'd read this to mean it's fine to indeed copy any module's definition out (?) to replace an instance. With or without public -- but with public it'll never be dead so just want to call this out a bit.

You're absolutely right that the first bullet, by allowing optimizing A using B's definition is essentially equivalent to allowing (at least in some senses) inlining B's definition entirely. 👍

Maybe we need some way to express/manage that the "identity" (and count) of certain things matter, as well as perhaps their position in some way. For example, probably don't want to be cloning registers around? But dunno.

Pinning this down would help a lot I think, and has been run into on other related fronts.

Anyway good points, personally I think this should be allowed (copying B into A) and if it's not we should figure out why precisely and pin that down.

[dtzSiFive]

Cool! Added some comments for discussion. Thanks!

[dtzSiFive]

It is a point of discussion if public modules should be allowed to be instantiated. This could work one of several ways if allowed:

1. A public, instantiated module is compiled as if it were an external module. I.e., all ports are marked don't touch and the same restrictions apply on uninferred widths and abstract reset.

2. A public, instantiated module is compiled with all optimizations allowed, but the ports have to follow one of the port lowering ABIs. This implies no uninferred widths and no abstract reset.

3. A public, instantiated module is compiled will all optimizations on and you get a Verilog file out, but you have no idea what the ports are until you compile it. 😅

IMO public modules should have their ports fixed -- no new ports, none can be changed, renamed, or removed. And scalarized convention for use by an extmodule somewhere, unless (somehow) indicated explicitly differently.

I'd suggest not talking about whether optimizations are "allowed" -- they're always allowed the question is what are the semantics of the language under which they operate (as they always must)?

IMO public should mean that we cannot do the sort of optimization that would allow us to:

1. Delete ports -- because we must assume they may have a live user elsewhere
2. Constant-prop /into/ the circuit (optimization proof obligation here being "port is driving to known value in all instantiation sites" -- we must assume such exist we cannot inspect and so conservatively may not optimize a public thing w.r.t its use within the design)

This is not the same as dontTouch -- which I'd summarize in practice as the strongest blocker (and IIRC "preserve name" was part of it?) similar to making the signal /itself/ public s.t it may be remotely written from anywhere at any time. Public visibility on a module (IMO, we're inventing it) shouldn't necessarily imply that.

For example, a public module driving constants to its ports must preserve all ports (for ABI and for reasons above) but any modules compiled along with that definition can optimize themselves with analysis results over the public module -- that is, if it drives 5 to port x then at instantiation sites it's an analysis fact usable for transformation that the port will always have value 5. That is, you can analyze "upwards" through "public" but not "downwards" (and transform for purposes of optimization using such analysis results).

In LLVM (IIRC) there's actually a thing called available_externally (https://llvm.org/docs/LangRef.html#linkage-types) which can be used to give a definition for an entity (here, module) such that it could be inlined or reasoned about where it's used BUT is not the actual definition (not suggesting we have that here, but same sort of idea except this IS the definition of the thing as well).

Does that make sense / WDYT?

---

Reading your comment again, I think I missed your focus above a little, but think the above is worth adding to the discussion.

In the above framing, I'd say a compiler is free to (basically) inline a public module into its instantiation sites (if we did compiler-driven inlining vs annotation-driven today, but point is it /could/ -- or if we had control of naming / private visibility we could clone a public module and specialize it for our use of it, only obligated to keep it and its tree alone).

Public modules may not be deduplicated, and depending on where things land re:"can share any modules" may mean no dedup across public instantiation trees (as that would cause sharing).

Public modules marked inline maybe is okay (above) but unlike today where it also implies "I do not expect to see this in my output" the public module must always be present in the output, because, that's its whole thing.

[dtzSiFive]

May be worth picking the syntax change from https://github.com/chipsalliance/firrtl-spec/pull/20/files#diff-fc53c821ad3e7a3caa0a87f0f46f48202fca3bc55dbb9e56fc34cb0c6ec3ead3R21 but haven't looked at impact with/without that locally yet.

[mwachs5]

Regarding @dtzSiFive comments:

Public modules may not be deduplicated, and depending on where things land re:"can share any modules" may mean no dedup across public instantiation trees (as that would cause sharing).

I agree that this should be the policy, i dont think that dedup is mentioned anywhere in the spec as even a possibility, but since a public module has to be in a module with the same name as the firrtl module that seems like a given (because otherwise its name would change)?

Public modules marked inline maybe is okay (above) but unlike today where it also implies "I do not expect to see this in my output" the public module must always be present in the output, because, that's its whole thin

How can you mark a module inline, vs an instance? In this current doc, since public modules can't be instantiated by anything, the concept of inlining is not applicable (and again the concept of inlining doesn't appear in this doc so far)

[dtzSiFive]

I agree that this should be the policy, i dont think that dedup is mentioned anywhere in the spec as even a possibility, but since a public module has to be in a module with the same name as the firrtl module that seems like a given (because otherwise its name would change)?

(Yeah, apologies, dedup is not something in spec and arguably shouldn't be-- just using as example to tease out the sort of thing I'm thinking public should imply, which sounds we're all in agreement on! And yes given what appears to be our agreed notions here, it should be "a given" for sure. Just making sure we're looking at things same way!)

How can you mark a module inline, vs an instance? In this current doc, since public modules can't be instantiated by anything, the concept of inlining is not applicable (and again the concept of inlining doesn't appear in this doc so far)

This is interesting. Well, in CIRCT we move all annotations on instances to modules: https://github.com/llvm/circt/blob/6b1f86c3609ffead02bd820148c593da7c876063/lib/Dialect/FIRRTL/FIRRTLAnnotationHelper.cpp#L236 .
And FWIW our inliner does not inline in this instance-path-sensitive sort of way, see llvm/circt#2790 . So, perhaps as an implementation detail or maybe a bug, in practice inline might as well be an annotation on the module.

And again, you're right and I was using behavior we know and rely on to describe what "public" would mean-- as you say if we can't instantiate them locally then there's no sense to marking it inline --instance or module should be at best a no-op if not illegal.

[mwachs5]

@dtzSiFive yes you are i think asking a lot of good questions that apply if we allow "modules that are deep within someone else's hierarchy" to be public, but i think this proposal as written is dodging all of those

[seldridge]

This has been updated to allow for public modules to be instantiated.

[mwachs5]

Suggested change

	`module`{.verilog}.) A public module is usable outside of the current circuit.
	`module`{.verilog}.) A public module, once lowered, will be usable outside of the context of the current circuit.

[mwachs5]

i realized upon re-reading this that we are talking about usability at the verilog level, unless we also mean usability at a hypothetical firrtl-linking level, which i think is not what we are trying to say.

[seldridge]

I think we're punting on FIRRTL linking and saying that this always happens at the Verilog level. This would be analogous to a C++ header file being enough to determine the name of a function that will be compiled by another process. There's no C++ "linking" per-se. However, for this to work there needs to be a paired FIRRTL extmodule that has the same name and ports. (Which is the same as the C++ case.)

[seldridge]

Public modules may not be deduplicated, and depending on where things land re:"can share any modules" may mean no dedup across public instantiation trees (as that would cause sharing).

A public module can be deduplicated, but only such that this preserves the public guarantees. I'm talking about replace one instance with another instance, not deleting deduplicated modules, though. I.e., I don't see why we should disallow changes to the instance hierarchy involving public modules just as we don't disallow changes that arise from directed or cost-driven inlining. Consider the following:

circuit Foo:
  public module Bar:
    output a: UInt<1>
    connect a, UInt<1>(0)
  public module Baz:
    output a: UInt<1>
    connect a, UInt<1>(0)
  public module Foo:
    output a: UInt<1>
    inst bar of Bar
    inst baz of Baz
    connect a, xor(bar.a, baz.a)

A legal compilation of this to Verilog must produce three modules: Foo.sv, Bar.sv, and Baz.sv. However, there are many legal compilations of the internals of Foo. All of the following are legal FIRRTL representations of Foo:

Constant propagation rips out all instances:

public module Foo:
  output a: UInt<1>
  connect a, UInt<1>(0)

Baz deduplicates into Bar and instantiations of Baz are replaced with Bar:

public module Foo:
  output a: UInt<1>
  inst bar of Bar
  inst baz of Bar
  connect a, xor(bar.a, baz.a)

Bar deduplicates into Baz and instances of Bar and replaced with Baz:

public module Foo:
  output a: UInt<1>
  inst bar of Baz
  inst baz of Baz

Public modules marked inline maybe is okay (above) but unlike today where it also implies "I do not expect to see this in my output" the public module must always be present in the output, because, that's its whole thing.

Agree. Today, this roughly means "always inline an instantiation of this module into its instantiator". This still compose with public. It means that the module has to be kept around. (This is just restating your observation.)

[dtzSiFive]

A public module can be deduplicated, but only such that this preserves the public guarantees. I'm talking about replace one instance with another instance, not deleting deduplicated modules, though

Very good point. Awesome. And also a non-public module could be dedup'd /into/ a public one, I think.

Thanks for going through my comments and very glad for this direction! 🎉

[dtzSiFive]

Is it intended non-main modules should be something a separate FIRRTL file can make use of via, say, extmodule?

Since there's no ability to specify the circuit name for an extmodule (the circuit the module is "imported" from?) ABI relying on circuit name across boundaries are unable to match up. In particular I mean the ref ABI in the text below. This is an issue an practice and would be good to fix as part of this.

Since we're requiring the public module name to be the verilog name , and only really "link" at Verilog level, it seems any namespacing provided by including the circuit name is not useful in practice as there would be a naming conflict of Verilog modules already (although I do like the idea, esp leaving door open for such).

While this ABI is clear about a single unit ("what is required/can be expected re:Verilog output"), it doesn't really say how a FIRRTL compiler is supposed to make use of this on the instantiation-side from a separate FIRRTL (or anything else wishing to make use of a FIRRTL-ABI-built "circuit"), so maybe there's some room here-- in addition to how they're used ("module name" needs to be "defname" in practice) it's also unspecified how they're gathered/included such that the macros are available reliably (left to build flow)... (not unreasonably so, bear with me please)...

One could imagine requiring telling the build flow/driver/linker+compiler that module X is from circuit Y or something (since build flow knows what circuits it built anyway, even if individual builds either don't have that information or we'd like to be able to change the definitions being used (so don't want to bake circuit name in the code)?), leaving circuit name in the filenames for co-located multiple implementations of a thing but macros are just ref_<module>_ as within any single "linked" design the module name must be unique.

Similar but maybe simpler possibility is to just drop the circuit portion of the ref ABI (ref_<module> prefix for files/macros), or if wishing backwards-compat with things built using ABI in current document, then treat all modules as being within their own circuit ref_<module>_<module>.

I'll think some more, maybe we should discuss a bit. Thanks.

[seldridge]

This makes a lot of sense. Thanks for writing this up. It took me a couple of days to process. 😅 😓

There's two things here:

1. To properly handle linking across a boundary, only the module name should be used. This implies that some extmodule Foo should match up to some public module Foo in another compilation or it's Verilog/VHDL equivalent. This shouldn't specify the circuit name as the circuit where this comes from is irrelevant. It's up to the user to figure this out and link it properly.
2. There is a namespacing problem with private modules. At minimum, these should have the circuit name included. At best, these should use some name mangling which cannot be linked from another FIRRTL compilation. E.g., Verilog allows for $ in module names, but the FIRRTL spec disallows this. We could use this fact to generate a unique private module name which cannot conflict with something else like <circuit>$<module>. It's not pretty, but would work. The complication here is that if the spec isn't silent about this, then it imposes an implicit private module ABI which could be used to link things silently (except that the private ABI can't be accessed from FIRRTL due to the use of $?).

WDYT?

[seldridge]

The only problem with using the bare module name without the circuit for this is that conflicts are high. clang/gcc avoid this on the C++ side with more complicated name mangling. We could do something like that that includes the names of the ports and their types in a mangled way. This would be super ugly... Perhaps its best to use the module name, but expect/encourage people to use names that are prefixed intelligently and/or use very descriptive names.

[seldridge]

I added language that makes things more "public module-centric" in ce8aa58. Primarily, this means that the circuit name has no real effect anymore. The only place I see that being used in in the implementation-defined convention for private modules.

E.g., consider the following circuit:

circuit Foo:
  module Baz:

  public module Bar:
    inst baz of Baz

This must produce a Verilog module called Bar in a file Bar.sv. Any additional stuff (ref files, bind files) are all now created using the name Bar and ignoring Foo. This then actually makes this portable to any other compilation referring to Bar as an extmodule.

If the compiler produces a Verilog module for FIRRTL module Baz, it should mangle its name to avoid a collision with another Verilog module. Some examples/ideas:

• Foo_Baz (simple, to the point)
• _Foo_Baz
• Foo$Baz (has the benefit of being legal in FIRRTL, but not in Verilog)

The primary benefit of using the circuit name here is that it is trivial to change without an effect on the rest of the design now. This then fills one of, though not all of, the rolls of the NestedPrefixModulesAnnotation.

[dtzSiFive]

This sounds great!! I don't know how to "manage" the versioning details here -- I'd kinda thought the ABI was mostly versioned orthogonal to the FIRRTL spec, so this change should be ABI v3 and v4 or something?

Whatever seems best, I'm not sure how important that is presently, or if I'm missing something.

Anyway I think everything you've changed and your solutions/answers look great and excited for this direction. Thanks! 🚀

[dtzSiFive]

Oh, looks like ABI is versioned with FIRRTL spec ? And v1/v2 are parts of that or something. So probably nevermind 👍.

[seldridge]

I'm glad this makes sense. Thanks for checking the details.

Yes, the ABI document is co-versioned with the FIRRTL spec. However, the actual ABI can have different "versions" within it. ABI v1/v2 is really "lowered aggregates" and "preserved aggregates". This gets a little complicated as maybe we should then do the same thing for the ref and group ABIs given that this patch makes a change to them. I think we can dodge this and change it with the co-versioned version.

I.e., if you are compiling FIRRTL less than version 3 then you should be generating the files with a certain format. If you are compiling FIRRTL version 4 or greater, then you should generate the new format. I would like to avoid this complexity in CIRCT and do a hard switch to version 4. If we have to, we can add in support for version 3 if there is a use case. However, I'm not planning to add it.

[darthscsi]

This should probably be either a general restriction on RWProbe (never access a port directly), or it should be allowed here and bounce wires/variables are created to work around verilog. This comment is predicated on "used to access a public module's port" meaning taking a probe of the port from inside the module.

[dtzSiFive]

If we clarify the difference, that would help yes, and related issue of whether either is expected to actually force the ports themselves necessarily or only have in some sense equivalent behavior.

This restriction (currently in the spec, just described here now) is to ensure a FIRRTL compiler can emit what's needed on /both/ sides of a module with forced ports, to give room for bounce wires or whatever, since "may be forced" is not part of the signature/ABI.

Emitting ports (and other signals) as variables would mostly eliminate the need for this restriction, as you mention, and we should clarify the difference between rwprobe'ing instance results vs the ports from inside the module so that on, e.g., extmodule's that may not have ports as variables, there's no surprises.

Sound good?

[mmaloney-sf]

What constitutes a "use"?

Is there an intuition for why this rule is in place?

[darthscsi]

I'm not sure we should drop the circuit name. More generally, this would require extmodules to be in a separate circuit and we would need to be able to instantiate modules in other circuits.

[dtzSiFive]

If we have the circuit name -- and no way to declare the circuit extmodules are in -- how does this work? That's why I think dropping circuit name is helpful, but mostly I don't understand what you're saying ("this would require" -- which are you saying requires that--including circuit or not?).

Are you saying you think we should put extmodules in separate circuit (so that we know what circuit-name the ABI will put their macro under? (and compatibility limits use of one compilation unit to "linking" against definitions of that module that use the "expected" circuit name, not any definition?)), or that we don't want to do that?

If so, how does one use a public module (that isn't top) with probes from another FIRRTL circuit? What does the extmodule look like and how do we find the right ABI bits to use? Thanks!

[darthscsi]

Or maybe an alternate path is to drop the name from the circuit? Does it really provide any value anymore?

Basically, the question is does the circuit name matter for anything anywhere? If it does, it seems we should embed it in the ABI so as to avoid name conflicts when one circuit uses names from another. This obviously doesn't happen now, but it seems like it should. Multiple public modules provides most of what you need to have libraries in a namespace. If it doesn't matter, then sure, drop it all over.

The "would require" thing is referring to specifying the circuit extmodules are in, if circuit is an ABI-affecting namespace.

[seldridge]

Let's drop it. (Do we go a step further and drop circuits entirely? Probably no, but it's an interesting consideration.) If we don't then it's a hint to the compilation on how to prefix or mangle. However, this is weird as it isn't exactly specified in the ABI.

If we instead remove the name then a prefix can be later added as an optional attribute of the circuit and done in a more exact ABI way.

Other questions/design points: can a circuit be nested or should we design with this in mind? This would make circuit more like a namespace and would probably require dropping the top level circuit. This would require circuit names.

[seldridge]

Circuit names are dropped in 1b7f50b. This also removes the redundancy between an annotation without a target and an annotation targeting the circuit.

[darthscsi]

same comment as before, groups_circuit_module seems safer.

[darthscsi]

Generally approve. Things can be cleaned up as we get experience with what works.