[FIRRTL] Domains: Operation Approach #8843

seldridge · 2025-08-12T03:50:22Z

This is an alternative approach to implementing FIRRTL domains from #8830.
This represents domains as additional operands to operations instead of
capturing which domain something is colored by in its type.

At present, the actual kind of a domain, e.g., if a domain is a clock
domain, a reset domain, or something else, is not linked to the underlying
domain values.

This PR is broken up into individual logical commits making the following
changes:

Add firrtl.domain
Add domain type
Add domains to ports
Add unsafe_domain_cast operation

uenoku

DomainInfoAttr is a 2d integer array which has port indices, correct? In that case how do we keep that invariant through the pipeline? Is it legal to define a port before Domain?

uenoku · 2025-08-20T20:21:31Z

test/Dialect/FIRRTL/round-trip.mlir

+firrtl.module @Domains(
+  in %A: !firrtl.domain,
+  in %B: !firrtl.domain,
+  in %a: !firrtl.uint<1> domains [%A],


Does this work with anonymous block arguments?

No, it does not. This probably needs to use an alternative code path when there is a block arg that doesn't have a name.

FWIW FIRRTL allows duplicate port names, previously had verifier that needed to be rolled back: #3976 .

Consider:

FIRRTL version 5.1.0 circuit Domains: domain ClockDomain: public module Domains: input a_b: Domain of ClockDomain input a : { b: String } output b: UInt<5> domains [a_b] invalidate b

With current PR this input errors:

oops.fir:5:10: error: 'firrtl.module' op domain information for domain port "a_b" must be a 'FlatSymbolRefAttr' public module Domains: ^ oops.fir:5:10: note: see current operation: "firrtl.module"() <{annotations = [], convention = #firrtl<convention scalarized>, domainInfo = [[], [], [0 : ui32]], layers = [], portAnnotations = [], portDirections = array<i1: false, false, true>, portLocations = [loc("oops.fir":6:11), loc("oops.fir":7:11), loc("oops.fir":8:12)], portNames = ["a_b", "a_b", "b"], portSymbols = [], portTypes = [!firrtl.domain, !firrtl.string, !firrtl.uint<5>], sym_name = "Domains"}> ({ ^bb0(%arg0: !firrtl.domain, %arg1: !firrtl.string, %arg2: !firrtl.uint<5>): %0 = "firrtl.invalidvalue"() : () -> !firrtl.uint<5> "firrtl.matchingconnect"(%arg2, %0) : (!firrtl.uint<5>, !firrtl.uint<5>) -> () }) : () -> ()

seldridge · 2025-08-20T20:56:09Z

DomainInfoAttr is a 2d integer array which has port indices, correct?

Yes, 2D integer array with port indices. Mechanically this is being enforced in a verifier for modules and instances (with slight tweaks between them). For modules, this must be either an empty ArrayAttr or an ArrayAttr of ArrayAttrs where the outer dimension is the number of ports. This is the same requirements for port annotations.

For instances, this is the same as modules, except it cannot be an empty ArrayAttr. This is also the same as port annotations on instances, though I don't think port annotations on instances are used.

In that case how do we keep that invariant through the pipeline?

Two ways (first is a non-way):

We don't. After the forthcoming EraseDomains pass, all domain information is removed and the domain info on modules and instances is empty.
We do enforce it and we will probably need either new operations or tracking a bit that we are after domain inference and we need stricter checks.

Is it legal to define a port before Domain?

Currently, no. This is only being checked in the FIRRTL parser and MLIR parser, though, not with a verifier. There's no actual problem with, though, just we've never allowed FIRRTL text to ever refer to an identifier that comes later. The underlying storage would allow this, though.

uenoku

Two ways (first is a non-way):
We don't. After the forthcoming EraseDomains pass, all domain information is removed and the domain info on modules and instances is empty.
We do enforce it and we will probably need either new operations or tracking a bit that we are after domain inference and we need stricter checks.

As you mentioned offline we anyway need to use insertPorts for port modifications so I agree that it's feasible to maintain the correct indexes. Regardless it would be nice to check and add tests for LowerTypes/LowerSignature doesn't mess up the domain port indices.

Another way to represent this is to (ab?)use inner symbols for domain ports and use them as identifiers instead of indexes. This will certainly avoid nasty index management though using inner symbols for non-hardware stuffs might be not appropriate though.

include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td

lib/Dialect/FIRRTL/FIRRTLOps.cpp

uenoku · 2025-08-22T03:01:21Z

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

           AnnotationArrayAttr:$annotations,
-           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers
+           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers,
+           DefaultValuedAttr<ArrayRefAttr, "{}">:$domainInfo


Allowing intrinsics to specify domains for operands/results make sense but certainly that will require special handling for each operation in infer domain (or maybe we can prepare op interface to specify domain inference). For now could you reject intrinsics with domain info in LowerIntrinsic or somewhere?

I think this is going to be the same as with external modules. They can define domains, but they need to be completely specified. Same thing for public modules, no inferred domains. WDYT?

Yes that makes sense .

lib/Dialect/FIRRTL/FIRRTLOps.cpp

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

lib/Conversion/FIRRTLToHW/LowerToHW.cpp

dtzSiFive

Here we go!

Left some feedback 👍 .

Does this need a connect operation? I suggest adding one that behaves similarly to define and propassign (static single connect).

Some interesting inputs to consider:

This should error (symbol used as domain is not domain):

FIRRTL version 5.1.0
circuit Domains:
  public module ClockDomain:

  public module Domains:
    input A: Domain of ClockDomain
    input b: UInt<5> domains [A]

LowerOpenAggs lowers the open bundle to put domain in its own port causing an error. Putting b in same domain twice is needed, FWIW.
Generally, where can "Domain" types be used? From what I'm seeing in the syntax, probably Domain is not meant to be valid in a port type if it's not top-level?

FIRRTL version 5.1.0
circuit Domains:
  domain ClockDomain:

  public module Domains:
    input A: Domain of ClockDomain
    input x: { y: Domain }
    output b: UInt<5> domains [A, A]
    invalidate b

Instantiation involving domains (this example doesn't make sense yet but just wanted to see an instance):

FIRRTL version 5.1.0
circuit Domains:
  domain ClockDomain:

  module Child:
    output B: Domain of ClockDomain
    input b: UInt<5> domains [B]

  public module Domains:
    input A: Domain of ClockDomain
    input a: UInt<5> domains [A]

    inst c of Child
    connect c.b, a

lib/Dialect/FIRRTL/FIRRTLOpInterfaces.cpp

lib/Dialect/FIRRTL/Transforms/SpecializeOption.cpp

dtzSiFive · 2025-08-27T15:32:54Z

test/Dialect/FIRRTL/round-trip.mlir

+firrtl.module @Domains(
+  in %A: !firrtl.domain,
+  in %B: !firrtl.domain,
+  in %a: !firrtl.uint<1> domains [%A],


FWIW FIRRTL allows duplicate port names, previously had verifier that needed to be rolled back: #3976 .

Consider:

FIRRTL version 5.1.0 circuit Domains: domain ClockDomain: public module Domains: input a_b: Domain of ClockDomain input a : { b: String } output b: UInt<5> domains [a_b] invalidate b

With current PR this input errors:

oops.fir:5:10: error: 'firrtl.module' op domain information for domain port "a_b" must be a 'FlatSymbolRefAttr' public module Domains: ^ oops.fir:5:10: note: see current operation: "firrtl.module"() <{annotations = [], convention = #firrtl<convention scalarized>, domainInfo = [[], [], [0 : ui32]], layers = [], portAnnotations = [], portDirections = array<i1: false, false, true>, portLocations = [loc("oops.fir":6:11), loc("oops.fir":7:11), loc("oops.fir":8:12)], portNames = ["a_b", "a_b", "b"], portSymbols = [], portTypes = [!firrtl.domain, !firrtl.string, !firrtl.uint<5>], sym_name = "Domains"}> ({ ^bb0(%arg0: !firrtl.domain, %arg1: !firrtl.string, %arg2: !firrtl.uint<5>): %0 = "firrtl.invalidvalue"() : () -> !firrtl.uint<5> "firrtl.matchingconnect"(%arg2, %0) : (!firrtl.uint<5>, !firrtl.uint<5>) -> () }) : () -> ()

include/circt/Dialect/FIRRTL/FIRRTLAttributes.td

lib/Dialect/FIRRTL/FIRRTLOps.cpp

seldridge · 2025-08-26T21:18:51Z

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

           AnnotationArrayAttr:$annotations,
-           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers
+           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers,
+           DefaultValuedAttr<ArrayRefAttr, "{}">:$domainInfo


I think this is going to be the same as with external modules. They can define domains, but they need to be completely specified. Same thing for public modules, no inferred domains. WDYT?

lib/Dialect/FIRRTL/FIRRTLOps.cpp

seldridge · 2025-08-27T23:35:43Z

Does this need a connect operation? I suggest adding one that behaves similarly to define and propassign (static single connect).

It will. Though that can come in a subsequent PR.

Some interesting inputs to consider:

This should error (symbol used as domain is not domain):

FIRRTL version 5.1.0
circuit Domains:
  public module ClockDomain:

  public module Domains:
    input A: Domain of ClockDomain
    input b: UInt<5> domains [A]

Fixed in b171092.

Generally, where can "Domain" types be used? From what I'm seeing in the syntax, probably Domain is not meant to be valid in a port type if it's not top-level?

I don't see a reason to not allow it on any module (or a corresponding instantiation). Domains need to be valid in any port, wire, or bundle. Support for bundles is not rolled out yet, but will be added.

Instantiation involving domains (this example doesn't make sense yet but just wanted to see an instance)

FIRRTL version 5.1.0
circuit Domains:
domain ClockDomain:

module Child:
output B: Domain of ClockDomain
input b: UInt<5> domains [B]

public module Domains:
input A: Domain of ClockDomain
input a: UInt<5> domains [A]
inst c of Child
connect c.b, a

Your example is correct and how it should work. This is not super interesting without the connection operator for domains. However, this will force the question on if you even have to connect it or if the connection can be inferred.

dtzSiFive · 2025-08-28T01:51:04Z

Your example is correct and how it should work. This is not super interesting without the connection operator for domains.

Awesome! I meant this was maybe weird but is just the first way I came up with to instantiate a child with domain ports, and wasn't meant to be anything valid or not. That wasn't the point.

I forgot to mention this had a verifier error at the time of review.

However, this will force the question on if you even have to connect it or if the connection can be inferred.

Whoa, fascinating!

seldridge · 2025-08-28T22:45:13Z

FWIW FIRRTL allows duplicate port names, previously had verifier that needed to be rolled back: #3976 .

I don't think this has been true for a while as the FIRRTL spec states how to handle the collisions (and provides an example of this exact case): https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#the-scalarized-convention The problem is that the type lowering passes don't properly implement the spec. The second a_b should be a_b_0 and CIRCT is relying on the Verilog emitter to do this uniquification when it should happen eagerly.

dtzSiFive · 2025-08-29T01:46:12Z

I meant FIRRTL dialect. I think we're agreeing, FIRRTL language itself is clear here. I agree with your assessment and that's why I added the verifier 😁👍. Might have been your idea, I'm not sure 😅. I think you made the clarification to the spec, thanks for that! There was an argument that it's inappropriate to ask passes to worry about uniquing names (or providing them at all) that seems worth a consideration but generally this is surprising behavior at the very least. Anyway until that's resolved there are concerns about round tripping if using their names, depending how they're grabbed (their ssa names should roundtrip, so if this works the same way we're good, I haven't checked).

…

On Thu, Aug 28, 2025, 5:45 PM Schuyler Eldridge ***@***.***> wrote: *seldridge* left a comment (llvm/circt#8843) <#8843 (comment)> FWIW FIRRTL allows duplicate port names, previously had verifier that needed to be rolled back: #3976 <#3976> . I don't think this has been true for a while as the FIRRTL spec states how to handle the collisions (and provides an example of this exact case): https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#the-scalarized-convention The problem is that the type lowering passes don't properly implement the spec. The second a_b should be a_b_0 and CIRCT is relying on the Verilog emitter to do this uniquification when it should happen eagerly. — Reply to this email directly, view it on GitHub <#8843 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYZWN5NJ7QXT4TTJHVEY4WT3P6BA7AVCNFSM6AAAAACDVFRVHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMZVGE2TSOBSGA> . You are receiving this because your review was requested.Message ID: ***@***.***>

seldridge · 2025-08-29T02:54:02Z

Looking at that example again, the error seems to be due to something unrelated to the FIRRTL Dialect name collision. This is actually just dropping the domain information of the domain port. This will trip the verifier, but it looks unrelated to naming.

seldridge · 2025-08-29T03:44:17Z

Looking at that example again, the error seems to be due to something unrelated to the FIRRTL Dialect name collision. This is actually just dropping the domain information of the domain port. This will trip the verifier, but it looks unrelated to naming.

Fixed in: 84f502f A port insertion utility was dropping the FlatSymbolRefAttr from a Domain type port when it should have passed it through. That commit also fixes issues identified with port insertion needing to not generate <<NULL ATTRIBUTE>> for ports. There's a messy discontinuity between the storage for PortInfo which tracks "no domains" as a null pointer and the underlying operation storage for domains which needs to be an empty ArrayAttr for this.

Add a new `firrtl.domain` op which defines the existence of a kind of domain. E.g., this can be used to declare abstract notions of a clock or reset domain. Signed-off-by: Schuyler Eldridge <[email protected]>

Add a type for representing a value that is a domain, e.g., a clock or reset domain. This is currnetly not creatable from any Chisel operations. Signed-off-by: Schuyler Eldridge <[email protected]>

Add domain information to all FIRRTL module and instance ports. Signed-off-by: Schuyler Eldridge <[email protected]>

Add a new operation, `firrtl.unsafe_domain_cast`, which is used to do unsafe conversions to domains. Conceptually, this acts like a node whose input has unknown domain type and result has a known domain type. This is not a generic type ascription, as the domain to which the input is casted does not affect the input at all. The domain type is only assigned to the result. Add a sipmle canonicalizer that will replace an `unsafe_domain_cast` which has no domains specified. This operation is a no-op and there's no point in keeping it around. Signed-off-by: Schuyler Eldridge <[email protected]>

uenoku

The IR design looks solid and good to me, great work! The change looks already large so I'm fine with merging the current state and incrementally fixing up remaining things.

In the follow could you add tests for Transforms to make sure these passes don't drop domain info? Also LowerTypes and LowerSignature could insert ports so the currently IR will be broken when there is a domain port in the middle of the ports. Could you open tracking issues for that?

uenoku · 2025-08-28T16:49:24Z

include/circt/Dialect/FIRRTL/FIRRTLStructure.td

           AnnotationArrayAttr:$annotations,
-           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers
+           DefaultValuedAttr<LayerArrayAttr, "{}">:$layers,
+           DefaultValuedAttr<ArrayRefAttr, "{}">:$domainInfo


Yes that makes sense .

lib/Dialect/FIRRTL/FIRRTLOps.cpp

uenoku · 2025-08-29T07:28:30Z

test/Dialect/FIRRTL/parse-basic.fir

    node b = cat()
+
+;// -----
+FIRRTL version 5.1.0


Just a question but what FIRRTL version do you target for domain?

I'd target this for 6.0. However, this is currently nextFIRVersion which is 5.1.0. I'll deal with updating all this once the FIRRTL spec version solidifies.

Change domain ports to include a mandatory domain sort/kind, e.g., a ClockDomain or a ResetDomain. Signed-off-by: Schuyler Eldridge <[email protected]>

Update port updating methods to work with domains. Signed-off-by: Schuyler Eldridge <[email protected]>

seldridge · 2025-08-30T03:58:13Z

In the follow could you add tests for Transforms to make sure these passes don't drop domain info? Also LowerTypes and LowerSignature could insert ports so the currently IR will be broken when there is a domain port in the middle of the ports. Could you open tracking issues for that?

Good idea. Right now, this is only suitable for firtool -parse-only, circt-opt, and circt-translate -export-firrtl. Tracking issue to fix the bugs with insertion/deletion (and thereby get this working with the rest of the pipeline) here: #8906

seldridge force-pushed the dev/seldridge/firrtl-domains-operation-approach branch 2 times, most recently from 7ca003f to e846e78 Compare August 12, 2025 03:55

seldridge force-pushed the dev/seldridge/firrtl-domains-operation-approach branch from f7706fe to c1de2b9 Compare August 20, 2025 17:07

seldridge mentioned this pull request Aug 20, 2025

[FIRRT] Domains: Types Approach #8830

Closed

seldridge marked this pull request as ready for review August 20, 2025 19:55

seldridge requested a review from darthscsi as a code owner August 20, 2025 19:55

seldridge requested review from dtzSiFive, fabianschuiki and rwy7 August 20, 2025 19:55

seldridge mentioned this pull request Aug 20, 2025

[FIRRTL] Add InferDomains pass #8877

Closed

uenoku reviewed Aug 20, 2025

View reviewed changes

uenoku reviewed Aug 22, 2025

View reviewed changes

dtzSiFive reviewed Aug 27, 2025

View reviewed changes

include/circt/Dialect/FIRRTL/FIRRTLStructure.td Outdated Show resolved Hide resolved

dtzSiFive reviewed Aug 27, 2025

View reviewed changes

lib/Conversion/FIRRTLToHW/LowerToHW.cpp Show resolved Hide resolved

dtzSiFive reviewed Aug 27, 2025

View reviewed changes

include/circt/Dialect/FIRRTL/FIRRTLAttributes.td Outdated Show resolved Hide resolved

seldridge commented Aug 27, 2025

View reviewed changes

seldridge added 4 commits August 29, 2025 21:55

[FIRRTL] Add firrtl.domain

9d6fc03

Add a new `firrtl.domain` op which defines the existence of a kind of domain. E.g., this can be used to declare abstract notions of a clock or reset domain. Signed-off-by: Schuyler Eldridge <[email protected]>

[FIRRTL] Add domain type

ec95829

Add a type for representing a value that is a domain, e.g., a clock or reset domain. This is currnetly not creatable from any Chisel operations. Signed-off-by: Schuyler Eldridge <[email protected]>

[FIRRTL] Add domains to ports

31dfa50

Add domain information to all FIRRTL module and instance ports. Signed-off-by: Schuyler Eldridge <[email protected]>

uenoku approved these changes Aug 30, 2025

View reviewed changes

[FIRRTL] Add domain sorts/kids with Domain ports

ce6f5e1

Change domain ports to include a mandatory domain sort/kind, e.g., a ClockDomain or a ResetDomain. Signed-off-by: Schuyler Eldridge <[email protected]>

seldridge force-pushed the dev/seldridge/firrtl-domains-operation-approach branch from 84f502f to df67311 Compare August 30, 2025 03:33

[FIRRTL] Update port methods for domains

7e543de

Update port updating methods to work with domains. Signed-off-by: Schuyler Eldridge <[email protected]>

seldridge force-pushed the dev/seldridge/firrtl-domains-operation-approach branch from df67311 to 7e543de Compare August 30, 2025 03:42

seldridge mentioned this pull request Aug 30, 2025

[FIRRTL] Fix port insertion/deletion involving domain info #8906

Closed

seldridge merged commit 7e543de into main Aug 30, 2025
7 checks passed

seldridge deleted the dev/seldridge/firrtl-domains-operation-approach branch August 30, 2025 19:36

[FIRRTL] Domains: Operation Approach #8843

[FIRRTL] Domains: Operation Approach #8843

Uh oh!

Conversation

seldridge commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uenoku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seldridge commented Aug 20, 2025

Uh oh!

uenoku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dtzSiFive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seldridge commented Aug 27, 2025

Uh oh!

dtzSiFive commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seldridge commented Aug 28, 2025

Uh oh!

dtzSiFive commented Aug 29, 2025 via email

Uh oh!

seldridge commented Aug 29, 2025

Uh oh!

seldridge commented Aug 29, 2025

Uh oh!

uenoku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seldridge commented Aug 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

seldridge commented Aug 12, 2025 •

edited

Loading

uenoku left a comment •

edited

Loading

dtzSiFive commented Aug 28, 2025 •

edited

Loading