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

[RTL] Allow declaring generated external modules #547

Merged
merged 17 commits into from
Mar 26, 2021

Conversation

darthscsi
Copy link
Contributor

External module type representing modules which will be generated by external/later processes from information embedded in the IR. Each module.generated points to a type and has a list of parameters for the generator.

Generator types are single-inheritance structures which list required metadata, have a (unique) descriptor, and a version.

An example use of these nodes will be lowering of firrtl.mem operations. These will lower to a module.generated op and a FIRRTL-Memory generator type. Later passes can lower to RTL or external tools can use the captured parameters to run external memory generators.

@teqdruid
Copy link
Contributor

teqdruid commented Feb 8, 2021

How does this help with your use case? Does it affect the generated SV? Is it a signal to call an external tool to generate some SV to include?

@lattner
Copy link
Collaborator

lattner commented Feb 10, 2021

This is used to enable progressive lowering through external generators, e.g. memory compilers.

There is another interesting layering question that this poses: when the rtl dialect is split out to the comb dialect, what should become of things like this and rtl.module? These are more like circt.module concepts that are the underlying infra for many layers of hardware.

include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
@teqdruid
Copy link
Contributor

What I was getting at is why is this generic rather than a set of more specific ops which are lowered using external generators rather than an attribute? Like a MemoryOp, an FSMOp (for FSM compilers), etc. This would enable defining a minimal abstraction which is more easily tied into SSA, e.g. MemoryOp would define width and depth attributes and appropriately specific args/results. This feels rather "JSON-y" to me.

@darthscsi
Copy link
Contributor Author

What I was getting at is why is this generic rather than a set of more specific ops which are lowered using external generators rather than an attribute?
This is something of an escape hatch. I fully expect that there will be some standard memory nodes in the future. This, however, captures flow specific and custom tooling without needing changes to the core IR. You could imagine an environment with many tools able to generate blocks, each being developed independently of Circt. This gives a simple way to bundle all the generator arguments in the IR without resorting to external files to store all the metadata.

@teqdruid
Copy link
Contributor

So it is JSON-y! I could see this maybe being useful, but don't we want to start with specific then add this later if necessary? I could see CIRCT developers abusing this instead of writing specific ops. Do you have a specific use case for this?

@lattner
Copy link
Collaborator

lattner commented Feb 12, 2021

Do you plan to merge the extern module change into this to reduce the diff? I can take a look at some point, just don't want to waste the time.

@darthscsi
Copy link
Contributor Author

Do you plan to merge the extern module change into this to reduce the diff?
It has been, I show only 6 files changed

Copy link
Collaborator

@lattner lattner left a comment

Choose a reason for hiding this comment

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

Thanks for splitting out the other diffs. I like the way this is going, but I have a few questions about the details (on top of the naming and formatting nits points out in the patch):

  1. I'm not sure how the "single inheritance" chain is supposed to work here. How do you imagine the compiler is going to resolve and process that chain?

  2. What is the version number for? What scheme should be used? Generators are inherently complicated and perhaps not versioned in any useful way (e.g. may just have a git hash or something). I am not sure how this will be used - if it is required, a string may be a better way to go than an integer.

  3. What is the "required attrs" list for? Are these something like the parameters that get passed into the generator to configure it?

  4. Can/should these modules be parameterizable like external modules?

@@ -138,6 +138,102 @@ def RTLModuleExternOp : RTLOp<"module.extern",
let parser = "return ::parse$cppClass(parser, result);";
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please manually wrap these lines to fit in 80 columns in this file. Clang format doesn't do tblgen :(

include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved

For example:
generator.type @MEMORY, "Simple-Memory", 1, "write_latency", "read_latency"
generator.type @FIRMEM (parent: @MEMORY), "FIRRTL-Memory", 1, "ports"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you have a version number, and what does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My assumption is the the definition of a schema for some generator will change over time and generators will support a variety of schemas. This could be signaled with names which included versioning sub-strings in an ad-hoc manor, or in a consistent general way. For example, FIRRTL memory nodes will have some schema today, and when that schema changes tomorrow, it would be nice for generators which lower those nodes to be able to handle both flavors/versions.

This cannot be solved by embedding the version as an attribute in the required parameter list as that wouldn't support multiple concurrent versions in the same file.

@lattner lattner changed the title RTL module type for generated modules [RTL] Allow declaring generated external modules Feb 15, 2021
@darthscsi
Copy link
Contributor Author

  1. I'm not sure how the "single inheritance" chain is supposed to work here. How do you imagine the compiler is going to resolve and process that chain?

A module instance is an instance of some schema node. The required attributes of the instance is the union of the required attributes listed for the inheritance chain. The verifier for the rtl.module.generated node checks this in this patch.

  1. What is the version number for? What scheme should be used? Generators are inherently complicated and perhaps not versioned in any useful way (e.g. may just have a git hash or something). I am not sure how this will be used - if it is required, a string may be a better way to go than an integer.

I've assumed that a particular schema for some generator kind will change over time and I've assumed tools will want backwards compatibility. E.g. a firrtl memory generator will want to handle today's front-end output, as well as tomorrow's improved/fixed flavor. This could be done in an ad-hoc way by embedding some type of versioning in the names, or we can provide a general mechanism. I chose the latter because to make this uniform.

  1. What is the "required attrs" list for? Are these something like the parameters that get passed into the generator to configure it?

Yes, required attributes are the schema which a rtl.module.generated must provide. These are the parameters to the generator. They are not module parameters, it is assumed that a generator can produce a parameterized module, thus I don't want to mix module parameters with generator parameters. I do, however, want to strongly type the generated nodes, hence the schema declaration.

  1. Can/should these modules be parameterizable like external modules?

No, see above.
This seems reasonable:

module.generated @name, @kind (stuff) -> (stuff) {generator params, parameters = {WIDTH = 64 : i64}}

That is, a generator takes some parameters and returns a module which it a parameterized RTL/verilog module.

@drom drom added this to the SiFive-1 milestone Mar 23, 2021
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for pushing on this. I'm really looking forward to how this will simplify the memory lowering.

Some comments throughout. My only major question is if the inheritance is justified for inclusion at this time or if we should feature creep this in the future. It wasn't immediately obvious to me what the gain that inheritance was providing.

include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
include/circt/Dialect/RTL/RTLStructure.td Outdated Show resolved Hide resolved
lib/Dialect/RTL/RTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/RTL/RTLOps.cpp Outdated Show resolved Hide resolved
lib/Dialect/RTL/RTLOps.cpp Outdated Show resolved Hide resolved
@darthscsi darthscsi merged commit e95e6f3 into main Mar 26, 2021
@darthscsi darthscsi added enhancement New feature or request HW Involving the `hw` dialect labels Mar 26, 2021
@darthscsi darthscsi deleted the dev/anlenhar/module_generated branch April 6, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request HW Involving the `hw` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants