[FIRRTL] Domain inference #9106

rwy7 · 2025-10-17T20:32:31Z

Add a domain inference pass.

Metadata

Closes [FIRRTL] Add InferDomains pass #8877, as this subsumes [FIRRTL] Add InferDomains pass #8877.

seldridge

I'll split this up into several reviews as I'm able to get through different parts. Here's some very high-level stuff.

Also, please include minimal tests of the end-to-end firtool behavior.

seldridge · 2025-11-03T21:19:41Z

include/circt/Dialect/FIRRTL/FIRRTLDeclarations.td

+    /// Return the number of ports for this instance.
+    size_t getNumPorts() {
+      return getNumResults();
+    }
+
    /// Return the port direction for the specified result number.
    Direction getPortDirection(size_t resultNo) {
      return direction::get(getPortDirections()[resultNo]);
    }

    /// Return the port name for the specified result number.
-    StringAttr getPortName(size_t resultNo) {
+    StringAttr getPortNameAttr(size_t resultNo) {
      return cast<StringAttr>(getPortNames()[resultNo]);
    }
-    StringRef getPortNameStr(size_t resultNo) {
-      return getPortName(resultNo).getValue();
+
+    StringRef getPortName(size_t resultNo) {
+      return getPortNameAttr(resultNo).getValue();
+    }
+
+    Location getPortLocation(size_t) {
+      return getLoc();


This cleanup for consistency of naming/functionality is great. If you so desire, land this directly/separately.

include/circt/Dialect/FIRRTL/Passes.td

seldridge · 2025-11-04T17:27:19Z

include/circt/Firtool/Firtool.h

Does this require updates to the C-API?

lib/Firtool/Firtool.cpp

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

seldridge

Second review (of three). This just looks at the tests and sees what the pass is supposed to be doing. Only nits, really. These look great.

seldridge · 2025-11-05T03:12:56Z

test/Dialect/FIRRTL/infer-domains-errors.mlir

+    // expected-error @below {{illegal "ClockDomain" crossing in port "p"}}
+    // expected-note  @below {{1st instance: A}}
+    // expected-note  @below {{2nd instance: B}}
+    in %p: !firrtl.uint<1> domains [%A, %B]


This error originally seemed incorrect to me. However, this is actually pretty clever and correct. Nice.

test/Dialect/FIRRTL/infer-domains-errors.mlir

seldridge · 2025-11-05T03:16:45Z

test/Dialect/FIRRTL/infer-domains-errors.mlir

+// Illegal domain crossing at matchingconnect op.
+firrtl.circuit "IllegalDomainCrossing" {
+  firrtl.domain @ClockDomain
+  firrtl.module @IllegalDomainCrossing(
+    in %A: !firrtl.domain of @ClockDomain,
+    in %B: !firrtl.domain of @ClockDomain,
+    // expected-note @below {{2nd operand has domains: [ClockDomain: A]}}
+    in %a: !firrtl.uint<1> domains [%A],
+    // expected-note @below {{1st operand has domains: [ClockDomain: B]}}
+    out %b: !firrtl.uint<1> domains [%B]
+  ) {
+    // expected-error @below {{illegal domain crossing in operation}}
+    firrtl.matchingconnect %b, %a : !firrtl.uint<1>
+  }
+}


(I'm reviewing backwards, so don't know the answer to this, yet:)

Are errors accumulated in a module or is exit immediate?

Nit: If not accumulated, we probably want that. If they are accumulated, it may be cleaner to combine this test with the previous one.

seldridge · 2025-11-05T19:27:14Z

test/Dialect/FIRRTL/infer-domains-errors.mlir

+    in D1 : !firrtl.domain of @ClockDomain,
+    // expected-note @below {{associated with "ClockDomain" port "D2"}}
+    in D2 : !firrtl.domain of @ClockDomain,
+    // expected-error @below {{ambiguous "ClockDomain" association for port "i"}}


Nit: This error is very similar to the DomainCrossOnPort test. However, the error message is very different. Would this be clearer/simpler as just Illegal domain crossing?

seldridge · 2025-11-05T19:29:54Z

test/Dialect/FIRRTL/infer-domains-errors.mlir

+    // expected-note @below {{candidate association "DO"}}
+    out %DO : !firrtl.domain of @ClockDomain,
+    in  %i  : !firrtl.uint<1> domains [%DO],
+    // expected-error @below {{ambiguous "ClockDomain" association for port "o"}}


For this "ambiguous" error message, it seems like we could suggest a fix to the user, e.g., "Use an unsafe domain cast to disambiguate". I really like the "ambiguous" messaging as it's just indicating that there are multiple solutions and the compiler can't figure it out for you, just like with certain type systems that require you to provide hints in certain situations, e.g., mandatory type annotations for recursive functions.

seldridge · 2025-11-05T19:58:38Z

test/Dialect/FIRRTL/infer-domains.mlir

+    out %uint_output: !firrtl.uint<8>,
+    out %sint_output: !firrtl.sint<4>


Missing CHECKs.

seldridge · 2025-11-05T20:13:45Z

test/Dialect/FIRRTL/infer-domains.mlir

+  )
+
+  firrtl.module @ExportDomain(
+    // CHECK: out %ClockDomain: !firrtl.domain of @ClockDomain


Getting a better name for this may be reasonable. Just "ClockDomain" is going to run into ClockDomain_0, etc. (Though, how many different inferred domains like this are going to exist?)

Possible something like domain_foo_A to give a hint as to its provenance.

seldridge · 2025-11-05T20:23:27Z

test/Dialect/FIRRTL/infer-domains.mlir

+  // CHECK: firrtl.module @InstanceUpdate(in %ClockDomain: !firrtl.domain of @ClockDomain, in %i: !firrtl.uint<1> domains [%ClockDomain]) {
+  // CHECK:   %foo_ClockDomain, %foo_i = firrtl.instance foo @Foo(in ClockDomain: !firrtl.domain of @ClockDomain, in i: !firrtl.uint<1> domains [ClockDomain])
+  // CHECK:   firrtl.domain.define %foo_ClockDomain, %ClockDomain
+  // CHECK:   firrtl.connect %foo_i, %i : !firrtl.uint<1>
+  // CHECK: }


Nit: I have a hard time reading these when the modules have multiple ports. Consider using CHECK-SAME to split the checks across multiple lines with indentation.

Ditto throughout.

seldridge · 2025-11-05T20:24:55Z

test/Dialect/FIRRTL/infer-domains.mlir

+  }
+}
+
+// CHECK-LABEL: ConstantInMultipleDomains


This test would be more readable if manually formatted to one-port-per-line.

Also, nice to see this working!

seldridge · 2025-11-05T20:26:22Z

test/Dialect/FIRRTL/infer-domains.mlir

+  }
+}
+
+firrtl.circuit "Top" {


What is this test checking?

seldridge

More partial review.

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

seldridge · 2025-11-05T23:17:40Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+/// Each declared domain in the circuit is assigned an index, based on the order
+/// in which it appears. Domain associations for hardware values are represented
+/// as a list of domains, sorted by the index of the domain type.
+using DomainTypeID = size_t;


The indirection to size_t was somewhat unexpected as opposed to using Operation * here (and sorting or using a set). I thought this was safe given that the underlying ops are never moved or changed, but that may not be true if any IR is modified. Do you know?

Or: is there an alternative that doesn't require the numbering here and can use something already available? (I realize that port IDs are used for associations. However, there's nothing else that can be used in that situation. 😅 )

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

seldridge · 2025-11-05T23:47:06Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+      auto *block = arg.getOwner();
+      auto *owner = block->getParentOp();
+      auto module = cast<FModuleOp>(owner);
+      auto info = module.getDomainInfoAttr();
+      auto i = arg.getArgNumber();
+      return getDomainTypeID(info, i);


Consider keeping the logical temporaries for info and i (and possibly owner) and inlining block and module. Long chains of temporaries that are only there to indicate "the next step" tend to hurt reading comprehension:

Suggested change

auto *block = arg.getOwner();

auto *owner = block->getParentOp();

auto module = cast<FModuleOp>(owner);

auto info = module.getDomainInfoAttr();

auto i = arg.getArgNumber();

return getDomainTypeID(info, i);

auto *owner = arg.getOwner()->getParentOp();

auto info = cast<FModuleOp>(owner).getDomainInfoAttr();

auto i = arg.getArgNumber();

return getDomainTypeID(info, i);

I do recognize that this is a fine balance between "inline everything" (which probably isn't the best here) and "inline enough so that a reader can follow it".

Ditto for the later instance case.

Mega-nit: Most syntax highlighters aren't smart enough to understand when module is a keyword and when it isn't. Consider avoiding this and using moduleOp when it comes up.

seldridge · 2025-11-05T23:53:19Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+    auto index = domainTable.size();
+    auto name = op.getNameAttr();
+    domainTable.push_back(op);
+    typeIDTable.insert({name, index});


Mega-nit: consider changing the order of modifications to domainTable and typeIDTable to move the temporaries closer to their single use sites.

Also, as always, consider inlining single use variables when it helps clarity (or using comments: {*/name=*/op.getNameAttr(), /*index=*/domainTable.size()}.

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

Add a pass that does domain inference and checking. This is used to verify the legality of a FIRRTL circuit with respect to its domains. E.g., this pass is intended to be used for checking for illegal clock domain crossings. Signed-off-by: Schuyler Eldridge <[email protected]>

…completeness

- instanceGraph.walkPostOrder - circuit debug scoped pass logger

seldridge

This is looking really great. I have not had time to actually play with it. I'd like to do this a bit more before landing it.

General comments:

There's a lot of use of Term * which could probably be Term &. Not super critical.
Try to avoid long chains of auto that are "recipes" for what the program is doing. It's difficult to know what is a true temporary vs. what is the individual steps to get to a temporary which is single-use. I tried to indicate places where this was confusing. Some ways to fix it: inline single-use, use /*foo=*/ comments for named args, use types instead of auto, etc.
The PR mixes some early exits with continuation to report all errors. For an inference pass, it seems like it is better to report everything that failed instead of ever exiting early.

I'm not following what all the options are doing for -domain-mode. Specifically, I expected the following to error with firtool -domain-mode check, however, it doesn't error. This only errors with -domain-mode infer-all:

FIRRTL version 6.0.0
circuit Foo:
  domain ClockDomain:
    period: Integer
  domain PowerDomain:
    voltage: Integer


  public module Foo:
    input ClockA: Domain of ClockDomain
    input ClockB: Domain of ClockDomain
    input PowerA : Domain of PowerDomain

    input a: UInt<1> domains [ClockA, PowerA]
    output b: UInt<1> domains [ClockB, PowerA]

    connect b, a

Can the PR add tests of the firtool end-to-end behavior, specifically with regard to the different -domain-mode options?

seldridge · 2025-11-06T23:08:26Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+struct VariableTerm : public TermBase<TermKind::Variable> {
+  VariableTerm() : leader(nullptr) {}
+  VariableTerm(Term *leader) : leader(leader) {}
+  Term *leader;
+};


This could skip the constructors and just use aggregate initializers instead:

Suggested change

struct VariableTerm : public TermBase<TermKind::Variable> {

VariableTerm() : leader(nullptr) {}

VariableTerm(Term *leader) : leader(leader) {}

Term *leader;

};

struct VariableTerm : public TermBase<TermKind::Variable> {

Term *leader;

};

VariableTerm foo{};

VariableTerm bar{term};

The LLVM coding guidelines don't seem to have an opinion here. However, it seems cleaner to not declare trivial constructors which are the same (I think / is foo{} going to use nullptr?) as the aggregate initializers.

Same comment for ValueTerm and RowTerm.

seldridge · 2025-11-06T23:10:57Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+struct VariableIDTable {
+  size_t get(VariableTerm *term) {
+    auto [it, inserted] = table.insert({term, table.size() + 1});
+    return it->second;
+  }
+
+  DenseMap<VariableTerm *, size_t> table;
+};


Does the table need to be public? If not, then class with private table would be better.

seldridge · 2025-11-06T23:49:38Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+  bool first = true;
+  for (auto *element : term->elements) {
+    if (!first)
+      out << ", ";
+    dump(out, element);
+    first = false;
+  }


Using llvm::interleaveComma is preferable.

seldridge · 2025-11-06T23:52:47Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+  if (!term)
+    return out << "null";
+  if (auto *var = dyn_cast<VariableTerm>(term))
+    return dump(out, var);
+  if (auto *val = dyn_cast<ValueTerm>(term))
+    return dump(out, val);
+  if (auto *row = dyn_cast<RowTerm>(term))
+    return dump(out, row);


Consider TypeSwitch as it's templating makes this very compact. (All of the cases can be collapsed to one case with a 3-entry parameter pack like .Case<VariableTerm, ValueTerm, RowTerm>([&](auto a){ dump(out, a); })

seldridge · 2025-11-07T00:10:29Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+  if (auto *var = dyn_cast<VariableTerm>(x)) {
+    if (var->leader == nullptr)
+      return var;
+
+    auto *leader = find(var->leader);
+    if (leader != var->leader)
+      var->leader = leader;
+    return leader;
+  }


Nit: consider inverting the match to allow for an early exit.

seldridge · 2025-11-11T01:55:19Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+        // The domain is defined internally. If there value is already exported,
+        // or will be exported, we are done.


Suggested change

// The domain is defined internally. If there value is already exported,

// or will be exported, we are done.

// The domain is defined internally. If the value is already exported,

// or will be exported, we are done.

seldridge · 2025-11-11T01:56:49Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+        auto portName = domainName;
+        auto portType = DomainType::get(module.getContext());
+        auto portDirection = Direction::Out;
+        auto portSym = StringAttr();
+        auto portLoc = port.getLoc();
+        auto portAnnos = std::nullopt;
+        auto portDomainInfo = FlatSymbolRefAttr::get(domainName);
+        PortInfo portInfo(portName, portType, portDirection, portSym, portLoc,
+                          portAnnos, portDomainInfo);


Nit: these can all be inlined and explained with comments:

PortInfo portInfo( /*portName=*/ domainName, ... );

Ditto below.

seldridge · 2025-11-11T02:08:28Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+  auto result = success();
+  module.getBodyBlock()->walk([&](Operation *op) -> WalkResult {
+    if (failed(updateOpDomainAssociations(op))) {
+      result = failure();
+      return WalkResult::interrupt();
+    }
+    return WalkResult::advance();
+  });
+  return result;


Can instead check if interrupted.

seldridge · 2025-11-11T02:09:36Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+    if (isa<DomainType>(type)) {
+      if (direction == Direction::In) {
+        bool driven = false;
+        for (auto *user : port.getUsers()) {
+          if (auto connect = dyn_cast<FConnectLike>(user)) {
+            if (connect.getDest() == port) {
+              driven = true;
+              break;
+            }
+          }
+        }
+        if (!driven) {
+          auto *term = getTermForDomain(port);
+          term = find(term);
+          if (auto *val = dyn_cast<ValueTerm>(term)) {
+            auto loc = port.getLoc();
+            auto value = val->value;
+            DomainDefineOp::create(builder, loc, port, value);
+          } else {
+            emitDomainPortInferenceError(op, i);
+            return failure();
+          }
+        }
+      }
+    }


The indentation is pretty deep here. Can this be cleaned up with condition inversion?

seldridge · 2025-11-11T02:14:57Z

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

+  if (auto extModule = dyn_cast<FExtModuleOp>(op)) {
+    return checkModuleDomains(globals, extModule);
+  }


Suggested change

if (auto extModule = dyn_cast<FExtModuleOp>(op)) {

return checkModuleDomains(globals, extModule);

}

if (auto extModule = dyn_cast<FExtModuleOp>(op))

return checkModuleDomains(globals, extModule);

rwy7 force-pushed the simple-infer-domains branch from d5a833c to ad4aa5d Compare November 4, 2025 17:16

seldridge reviewed Nov 4, 2025

View reviewed changes

rwy7 force-pushed the simple-infer-domains branch from ad4aa5d to 565cd27 Compare November 4, 2025 18:26

seldridge reviewed Nov 5, 2025

View reviewed changes

seldridge reviewed Nov 6, 2025

View reviewed changes

seldridge and others added 5 commits November 6, 2025 13:21

InferDomains: add infer-public flag, check interfaces of modules for …

13891f2

…completeness

Add domains option

22a7dbc

Fix tests

f5daa23

Review comments

66e46be

rwy7 force-pushed the simple-infer-domains branch from 565cd27 to 66e46be Compare November 6, 2025 18:43

rwy7 added 3 commits November 6, 2025 14:27

Update comments

c55d9e9

Remove unused typedefs

8164589

Review comments

ab0c6f0

- instanceGraph.walkPostOrder - circuit debug scoped pass logger

seldridge reviewed Nov 11, 2025

View reviewed changes

		out %uint_output: !firrtl.uint<8>,
		out %sint_output: !firrtl.sint<4>

		// The domain is defined internally. If there value is already exported,
		// or will be exported, we are done.

[FIRRTL] Domain inference #9106

Are you sure you want to change the base?

[FIRRTL] Domain inference #9106

Uh oh!

Conversation

rwy7 commented Oct 17, 2025 • edited by seldridge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Metadata

Uh oh!

seldridge 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!

Uh oh!

Uh oh!

seldridge 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seldridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

seldridge 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!

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!

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!

Reviewers

rwy7 commented Oct 17, 2025 •

edited by seldridge

Loading