Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Add public modules #130
[major] Add public modules #130
Changes from all commits
c7292b3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 circuitref_<module>_<module>
.I'll think some more, maybe we should discuss a bit. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
extmodule Foo
should match up to somepublic 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.$
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
This must produce a Verilog module called
Bar
in a fileBar.sv
. Any additional stuff (ref files, bind files) are all now created using the nameBar
and ignoringFoo
. This then actually makes this portable to any other compilation referring toBar
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, looks like ABI is versioned with FIRRTL spec ? And v1/v2 are parts of that or something. So probably nevermind 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this 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 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circuit names are dropped in 1b7f50b. This also removes the redundancy between an annotation without a target and an annotation targeting the circuit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before, groups_circuit_module seems safer.