[FIRRTL] Add a convention attribute to modules #4665

rwy7 · 2023-02-16T02:05:20Z

The convention attribute give us module-by-module control over how a module's ports should be lowered. When a module uses the "scalarized" convention, its ports are guaranteed be fully scalarized in the output verilog. When a module uses the "internal" convention, then it's ports will be lowered according to the configured aggregate preservation mode.

A module's convention is internal by default, but can be set using a new annotation, or through the firtool command line options --scalarize-top-module and --scalarize-ext-modules.

Enabling both --scalarize-top-module and --scalarize-ext-modules together (which is the default) should be a drop-in replacement of the older --preserve-public-types option, which has been removed.

Changes

FIRRTL Dialect
- add new convention attribute to FModuleLike objects. This property is either scalarized or internal.
- The convention controls how a module's ports are lowered by LowerTypesPass.
- FModule and FExtModule have new builders that let you pass a convention through. Otherwise, they default to internal convention.
- FMemModule always uses the internal convention, for now.
- In the printed format, the convention is printed in the attribute-dict, but only if it's set to scalarized.
lowerTypes
- when a module has the scalarized convention, the pass must scalarize it's ports, regardless of the aggregate preservation mode.
- when a module has the internal convention, the pass lowers ports based on the configured aggregate preservation mode.
- The preservePublicTypes option is removed.
Firtool
- remove preserve-public-types option
- add two new options, --scalarize-top-module and --scalarize-ext-modules.
- both default to on.
- when --scalarize-top-module is enabled, FIRParser will mark the main module with the scalarized convention. Otherwise, the top module will use the internal convention.
- when --scalarize-ext-modules is enabled, FIRParser will mark any ext modules with the scalarized convention.
- An external top module will be scalarized if either option is enabled.
Annotations:
- add a new annotation called firrtl.module.Convention (please suggest a better name!)
- allows the convention to be specified through annotations.
- conventions applied in lowerAnnotations.
- Because annotations are applied later in the pipeline, annotations takes precedence over any scalarization option passed to firrtool.

rwy7 · 2023-02-16T02:07:30Z

include/circt/Dialect/FIRRTL/AnnotationDetails.h

@@ -28,6 +28,8 @@ constexpr const char *rawAnnotations = "rawAnnotations";
 // Annotation Class Names
 //===----------------------------------------------------------------------===//

+constexpr const char *conventionAnnoClass =
+    "firrtl.linkage.Convention";


@seldridge @jackkoenig I'm adding this annotation to mfc before adding it to chisel. Any suggestions on a good class name?

mwachs5 · 2023-02-16T03:17:52Z

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

+
+| Property   | Type   | Description                 |
+| ---------- | ------ | --------------------------- |
+| class      | string | `firrtl.linkage.Convention` |


do we want this to be the level of the annotation, or do we want something more blanket firrtl.annotations.PublicModule?

which will act as a proxy for a public module that might get added to firrtl spec?

test/Dialect/FIRRTL/lower-types-aggregate.mlir

seldridge

High level: this seems fine.

Providing more fine-grained control and doing it by adding a per-module/extmodule attribute (controlled by an annotation that is immediately removed) seems sound.

I don't know what to call this. "Linkage" seems a bit esoteric as opposed to something more pedestrian like: "Lowering Style".

There is an alternative of going more directly into a single annotation, "Public", which controls if module ports are lowered or not. That precludes more fine-grained lowering styles (it's all or nothing) which we don't have a use case for today.

seldridge · 2023-02-16T03:08:47Z

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

-            (ins DefaultValuedAttr<AnnotationArrayAttr, "{}">:$annotations);
+  let arguments = (ins
+    ConventionAttr:$convention,
+    DefaultValuedAttr<AnnotationArrayAttr, "{}">:$annotations


Does declaring this with a default value of "internal" simplify some of the parsing logic later around checking for an empty attribute?

As far as I can tell, DefaultValuedAttr doesn't help here, because we skip default builders and we have a custom asm printer/parser.

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

test/Dialect/FIRRTL/annotations.mlir

test/firtool/convention.fir

youngar · 2023-02-16T04:41:26Z

There is an alternative of going more directly into a single annotation, "Public", which controls if module ports are lowered or not.

The problem with the public/private concept, is that it brings a lot of baggage with it. We can't constant prop across a public module's ports, or delete any ports, have uninferred widths or resets, or change anything about the module's interface. This is a problem for our use case in unit testing; we want to be able to test modules with uninferred resets.

public/private primarily is a statement of visibility between compilation units, which is distinct from the convention used. I think we could eventually have many different supported conventions (e.g. bundle splitting on flow, 1d-vector-preservation). I also think, very soon, we will want to be able to specify the visibility/linkage of a module (public vs private) in addition to it's convention, and that these two orthogonal mechanisms can work very well together.

It might be interesting to take a look at LLVM's linkage and calling convention support and how they work together.

uenoku · 2023-02-16T08:39:12Z

lib/Dialect/FIRRTL/FIRRTLOps.cpp

  buildModule(builder, result, name, ports, annotations);
+  result.addAttribute("convention", convention);


Can we add "convention" attribute within buildModule function?

This code is out here because mem modules don't have a settable convention attribute, but still build using the buildModule helper. If mem modules grow a convention attr, we can share mode code, but I am punting on mem modules for this PR.

There are equivalent concerns for mem modules, but the implementation is burried in annotations and multiple passes. An issue, at least, should be filed for unifying memory module lowering with this. (e.g. memories may flatten types, they may split up memories on field boundaries, etc).

uenoku · 2023-02-16T10:10:00Z

Separating linkage and visibility make sense. If the convention attribute is mandatory, can we implement custom parser instead of using attr-dict?

firrtl.module private internal @Foo(..) {

Also what's expected behaivor of "public internal" module?

dtzSiFive · 2023-02-16T14:42:51Z

Separating visibility from calling convention sounds good to me, especially if we want to change one and not the other (as you say). Even if some go together, nice to separate the concepts, love the direction of this PR.

Just to be clear, calling convention is usually not about a changed (high-level) interface/signature, but the mechanisms used to implement it (e.g., pass aggregates as such or scalarized (this PR), or pass arguments on stack vs register and how/where). Which may be explicitly modeled during lowering or in the backend IR, of course.
But so I'd expect the corresponding extmodule declaration for a module to be the aggregate version with the appropriate convention on it, where the scalarized version is a lower-than-FIRRTL implementation detail. Similarly, the convention (at least in LLVM) must match across caller/callee.

If this idea works differently (perhaps a documented guaranteed equivalence between one signature with one convention and another with the known-to-be-compatible signature, and what that means?), it'd be good to specify this. Put another way: Which signatures are legal to use with which signatures, across the pairs of calling conventions. Do we anticipate any calling conventions that cannot be represented with equivalent signatures at the FIRRTL level?

Little unsure how this helps the mentioned driving use case, however:

This is a problem for our use case in unit testing; we want to be able to test modules with uninferred resets.

Interesting! Are these tests going to be built as part of the design, then, or do we need a new (visibility?) variety that captures what it seems is being asked for here: expose the module for visibility (symbol-wise), but loosen (some?) of the interface-must-not-change requirements (allowing const-prop, uninferred resets)? "visible but safe to optimize as if it wasn't"? Can ports be removed, must widths be inferred? (What happens if a constant is propagated through but a unit test drives using a different value?)
Offhand I'd expect private modules not to be guaranteed to have a particular name (especially when linking, if we had such scopes/namespaces), but I think in practice we try to do this anyway. So maybe it'd be good to write down what we want from each of these things.

From a different angle, since maybe there's missing context: how does a unit test interact with these, I guess in a "white box"-style approach somehow whereby it's okay to not know the interface of the module under test (until it's been built to a lower form)? Does it expect to "link" from an interface w/an uninferred reset to an implementation with resets (/widths/etc.?) inferred?
(Or is that detail re:resets specifically significant since data-wise they're the same and the responsibility of using an appropriate reset is on the test author?)

Maybe we should discuss some of this offline 😇 .

Separating linkage and visibility make sense

EDIT: Oh, I see, the proposed annotation is calling it the linkage convention. Hmm.

nit: Calling convention and linkage are not the same thing, and I don't think this is about linkage. Calling convention seems like a good fit, IMO. Whether what we have now re:public/private is more visibility or linkage is hard to say, since we don't have a notion of "linking" yet-- much of the linkage types is specifying how things are resolved/combined when linked together (and how that's indicated in the output file format)., so I'm inclined to call it visibility for now until linking is a thing. Linkage concept for us, might specify whether two modules linked with the same name can be "deduplicated" or if having the same name is a collision, or if they're "hidden" and can be renamed, so on. I don't think we likely want things like weak or appending 😄 , at least not for modules. Hopefully we won't grow the complexity of LLVM's linkage 😄 .

rwy7 · 2023-02-16T15:26:56Z

In a few places I was using linkage to refer to both visibility and calling convention together, despite knowing better (in my mind, linkage is visibility + details). I was trying to put a word to "how and when and module can be instantiated", and misappropriated the word. I pushed a few changes to align on "module instantiation convention" and "visibility".

rwy7 · 2023-02-16T15:32:11Z

My intent is that the convention at the instantiation site would exactly match the target module's convention, just as you say Will, but it is interesting that for some signatures, different conventions might result in compatible lowerings... Something to meditate on I guess? Anyways, I haven't added a convention attribute to the instance op in this PR.

seldridge · 2023-02-16T15:33:42Z

Alright, I'm onboard with the linkage description. This is very, very related to the concept that extern is providing for C++ and more generally of linkage. Section 6.6 of the C++ spec outlines this and I think we are moving towards needing a "linkage" section of the FIRRTL spec which defines all this and replaces the limited descriptions about lowering of "public" things in the "compiler implementation details" section.

Generally, I think "linkage" is the right way to think about this. Public/private was something that we kind of invented for FIRRTL and I agree it doesn't align correctly with the concept here. We've gotten into issues with this before where we use a public symbol to mean one thing which runs against what normal MLIR conventions would mean.

rwy7 · 2023-02-16T15:53:59Z

Also what's expected behaivor of "public internal" module?

That would be a module that is public, and uses the internal calling convention / instantiation convention.

In the future, having a public module with internal convention is probably too dangerous to allow, because the lowering of internal convention changes depending on compiler flags, which may vary across compilation units. For now though, I think it's okay to allow.

dtzSiFive · 2023-02-16T16:41:20Z

I think modelling calling convention separately from linkage makes sense, but alright. They're still not the same thing 😄 , even if not all combinations are possible or one implies certain types of the other. Language linkage can imply a certain calling convention, although FWIW that's not what 6.6 is about, it's very clearly about how things link together and in what namespaces they reside? 9.11 (in n4917 anyway) does have this note:

Note 1 : Some of the properties associated with an entity with language linkage are specific to each implementation
and are not described here. For example, a particular language linkage might be associated with a particular form of
representing names of objects and functions with external linkage, or with a particular calling convention, etc. — end
note

So if by extern you meant extern "C" and others, sure (language linkage (9.11) != linkage (6.6)).

Mostly just want to be sure we're clear what this is. Calling it linkage works for me, and doesn't need to map to LLVM or C++ concepts other than it helps to re-use concepts folks know. Getting this into the FIRRTL spec sounds great. I was a bit off thinking this was meant to be calling convention alone, when it appears to be a combination of the two (+visibility(?), which is reasonably part of linkage as mentioned).

What makes it possible for modules to be visible externally (for unit testing) without inferred resets? Is that a new (not included) "linkage" kind? Or is the plan to have unit-tests target private (for now?) modules that have the unit-test-compatible linkage type (which in this PR seem about whether types are lowered, not whether widths/resets/etc are inferred?).

Don't mean to say it needs to all be figured out before moving forward, just want clear and consistent concepts. Ideally with the context needed to understand this included or available.

Anyway this is getting away from the details of this PR, which I think seems great. The utility of this for unit-testing use-case is unclear, but that is perhaps a discussion to be had elsewhere.

dtzSiFive

Some minor suggestions.

dtzSiFive · 2023-02-16T16:52:40Z

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

+
+```json
+{
+  "class": "firrtl.linkage.Convention",


This doesn't match the documentation above. However it's called, they should match. Sorry for the noise on re:feedback.

Thanks yes, totally forgot to change these. I promise to eventually eradicate the word linkage from this PR. I still need a real class name that we can use in chisel, suggestions anyone?

dtzSiFive · 2023-02-16T16:53:20Z

include/circt/Dialect/FIRRTL/AnnotationDetails.h

@@ -28,6 +28,7 @@ constexpr const char *rawAnnotations = "rawAnnotations";
 // Annotation Class Names
 //===----------------------------------------------------------------------===//

+constexpr const char *conventionAnnoClass = "firrtl.linkage.Convention";


(similarly, re:matching everywhere)

lib/Dialect/FIRRTL/FIRRTLOps.cpp

dtzSiFive · 2023-02-16T16:55:39Z

lib/Dialect/FIRRTL/FIRRTLOps.cpp

-  return parseFModuleLikeOp(parser, result, /*hasSSAIdentifiers=*/true);
+  if (parseFModuleLikeOp(parser, result, /*hasSSAIdentifiers=*/true))
+    return failure();
+  if (!result.attributes.get("convention"))


This is how this is done elsewhere, but consider hoisting the attribute name to some global location.

This annotation is being handled in mfc via: llvm/circt#4665 The convention lets users define how ports with aggregate types are lowered to verilog.

darthscsi · 2023-02-21T16:13:26Z

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

+| ---------- | ------ | --------------------------- |
+| class      | string | `firrtl.module.Convention`  |
+| convention | string | `scalarize` or `internal`   |
+| target     | string | Reference target            |


It seems like one should have target strings for modules, but the Chisel way is to have this information on instances. @seldridge should comment on implementability on the chisel side.

darthscsi · 2023-02-21T16:14:21Z

docs/Dialects/FIRRTL/FIRRTLAnnotations.md

+| Property   | Type   | Description                 |
+| ---------- | ------ | --------------------------- |
+| class      | string | `firrtl.module.Convention`  |
+| convention | string | `scalarize` or `internal`   |


is internal necessary? Should the default be "whatever we feel like" and only deviations from that be explicit?

dropped internal 👍

darthscsi · 2023-02-21T16:21:19Z

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp

@@ -442,31 +436,26 @@ struct TypeLoweringVisitor : public FIRRTLVisitor<TypeLoweringVisitor, bool> {
  // Cache some attributes
  const AttrCache &cache;

-  // Keep track of public modules.
-  const llvm::DenseSet<FModuleLike> &publicModuleSet;
+  const llvm::DenseMap<FModuleLike, Convention> &conventionTable;


Instead of a side map, can't you just ask the module when you need to know?

Since this pass is mutating module annotations concurrently across multiple threads, accessing another module's attributes directly can crash mfc. In an earlier issue, @uenoku had to switch to a side-table to track visibility, for this reason.

I thought on this pass the module op mutation was done serially and only the module bodies done in parallel.

Ah, yes, it was depending on parallel multation resulting in the same type. We've generally split module update form module body update to solve this, but it's fine for now. When module type lowering is split out to it's own pass, this whole pass can become a fmodule pass instead of a circuit pass.

darthscsi · 2023-02-21T16:22:15Z

lib/Dialect/FIRRTL/Transforms/LowerTypes.cpp


  // Set true if the lowering failed.
  bool encounteredError = false;
 };
 } // namespace

 /// Return aggregate preservation mode for the module. If the module has a
-/// public linkage, then it is not allowed to preserve aggregate values on ports
-/// unless `preservePublicTypes` flag is disabled.
+/// scalarized linkage, then we may not preserve it's aggregate ports.
 PreserveAggregate::PreserveMode
 TypeLoweringVisitor::getPreservatinoModeForModule(FModuleLike module) {


Specifically, here: module.getConvention();

This annotation is being handled in mfc via: llvm/circt#4665 The convention lets users define how ports with aggregate types are lowered to verilog.

darthscsi

LGTM.

This annotation is being handled in mfc via: llvm/circt#4665 The convention lets users define how ports with aggregate types are lowered to verilog.

rwy7 added the FIRRTL Involving the `firrtl` dialect label Feb 16, 2023

rwy7 commented Feb 16, 2023

View reviewed changes

rwy7 requested review from uenoku, seldridge and darthscsi February 16, 2023 02:07

rwy7 force-pushed the lowering-conventions branch 3 times, most recently from 1aa91f4 to 9786ab7 Compare February 16, 2023 03:09

mwachs5 reviewed Feb 16, 2023

View reviewed changes

test/Dialect/FIRRTL/lower-types-aggregate.mlir Outdated Show resolved Hide resolved

seldridge reviewed Feb 16, 2023

View reviewed changes

rwy7 force-pushed the lowering-conventions branch from 42ac8cb to a8c6449 Compare February 16, 2023 03:57

uenoku reviewed Feb 16, 2023

View reviewed changes

rwy7 force-pushed the lowering-conventions branch 4 times, most recently from 36a7bdd to 44215b5 Compare February 16, 2023 15:21

dtzSiFive reviewed Feb 16, 2023

View reviewed changes

rwy7 force-pushed the lowering-conventions branch from 44215b5 to f5f7204 Compare February 17, 2023 19:57

rwy7 mentioned this pull request Feb 17, 2023

Add an annotation for specifying module port conventions chipsalliance/chisel#3030

Merged

16 tasks

darthscsi reviewed Feb 21, 2023

View reviewed changes

rwy7 force-pushed the lowering-conventions branch 4 times, most recently from d6a09e8 to 6be2874 Compare March 3, 2023 14:41

rwy7 force-pushed the lowering-conventions branch from 6be2874 to 8f4aed4 Compare March 13, 2023 15:26

darthscsi approved these changes Mar 13, 2023

View reviewed changes

rwy7 force-pushed the lowering-conventions branch from 8f4aed4 to cf4f0b7 Compare March 16, 2023 18:13

[FIRRTL] Add a convention attribute to modules

acd0945

rwy7 force-pushed the lowering-conventions branch from cf4f0b7 to acd0945 Compare April 3, 2023 18:23

rwy7 merged commit b55e276 into llvm:main Apr 3, 2023

rwy7 deleted the lowering-conventions branch April 13, 2023 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Add a convention attribute to modules #4665

[FIRRTL] Add a convention attribute to modules #4665

rwy7 commented Feb 16, 2023 •

edited

Loading

rwy7 Feb 16, 2023

mwachs5 Feb 16, 2023

seldridge left a comment

seldridge Feb 16, 2023

rwy7 Feb 16, 2023

youngar commented Feb 16, 2023 •

edited

Loading

uenoku Feb 16, 2023 •

edited

Loading

rwy7 Feb 16, 2023

darthscsi Feb 21, 2023

uenoku commented Feb 16, 2023

dtzSiFive commented Feb 16, 2023 •

edited

Loading

rwy7 commented Feb 16, 2023

rwy7 commented Feb 16, 2023

seldridge commented Feb 16, 2023

rwy7 commented Feb 16, 2023

dtzSiFive commented Feb 16, 2023

dtzSiFive left a comment

dtzSiFive Feb 16, 2023

rwy7 Feb 16, 2023

dtzSiFive Feb 16, 2023

dtzSiFive Feb 16, 2023

darthscsi Feb 21, 2023

darthscsi Feb 21, 2023

rwy7 Feb 28, 2023

darthscsi Feb 21, 2023

rwy7 Feb 22, 2023

darthscsi Feb 22, 2023

darthscsi Feb 22, 2023

darthscsi Feb 21, 2023

darthscsi left a comment

		buildModule(builder, result, name, ports, annotations);
		result.addAttribute("convention", convention);

[FIRRTL] Add a convention attribute to modules #4665

[FIRRTL] Add a convention attribute to modules #4665

Conversation

rwy7 commented Feb 16, 2023 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngar commented Feb 16, 2023 • edited Loading

uenoku Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku commented Feb 16, 2023

dtzSiFive commented Feb 16, 2023 • edited Loading

rwy7 commented Feb 16, 2023

rwy7 commented Feb 16, 2023

seldridge commented Feb 16, 2023

rwy7 commented Feb 16, 2023

dtzSiFive commented Feb 16, 2023

dtzSiFive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darthscsi left a comment

Choose a reason for hiding this comment

rwy7 commented Feb 16, 2023 •

edited

Loading

youngar commented Feb 16, 2023 •

edited

Loading

uenoku Feb 16, 2023 •

edited

Loading

dtzSiFive commented Feb 16, 2023 •

edited

Loading