Add domains option

Author: rwy7

include/circt/Dialect/FIRRTL/Passes.h

@@ -69,6 +69,26 @@ enum class CompanionMode {
   Drop,
 };
 
+/// The mode for the InferDomains pass.
+enum class InferDomainsMode {
+  /// Check domains with inference for private modules (default).
+  Infer,
+  /// Check domains without inference.
+  Check,
+  /// Check domains with inference for both public and private modules.
+  InferAll,
+};
+
+/// True if the mode indicates we should infer domains on public modules.
+constexpr bool shouldInferPublicModules(InferDomainsMode mode) {
+  return mode == InferDomainsMode::InferAll;
+}
+
+/// True if the mode indicates we should infer domains on private modules.
+constexpr bool shouldInferPrivateModules(InferDomainsMode mode) {
+  return mode == InferDomainsMode::Infer || mode == InferDomainsMode::InferAll;
+}
+
 #define GEN_PASS_DECL
 #include "circt/Dialect/FIRRTL/Passes.h.inc"
 

include/circt/Dialect/FIRRTL/Passes.td

@@ -920,10 +920,18 @@ def InferDomains : Pass<"firrtl-infer-domains", "firrtl::CircuitOp"> {
     E.g., this pass can be used to check for illegal clock-domain-crossings if
     clock domains are specified for signals in the design.
   }];
-  let options = [
-    Option<"inferPublic", "infer-public", "bool", "false",
-      "Infer domains on public modules.">
-  ];
+  let options = [Option<"mode", "mode", "InferDomainsMode",
+                        "InferDomainsMode::Infer", "infer, check, infer-all.",
+                        [{
+                 llvm::cl::values(
+                   clEnumValN(InferDomainsMode::Infer, "infer",
+                     "Check domains with inference for private modules"),
+                   clEnumValN(InferDomainsMode::Check, "check",
+                     "Check domains without inference"),
+                   clEnumValN(InferDomainsMode::InferAll, "infer-all",
+                     "Check domains with inference for both public and private "
+                     "modules"))
+      }]>];
 }
 
 def LowerDomains : Pass<"firrtl-lower-domains", "firrtl::CircuitOp"> {

include/circt/Firtool/Firtool.h

@@ -23,6 +23,34 @@
 
 namespace circt {
 namespace firtool {
+
+enum class DomainMode {
+  /// Disable domain checking.
+  Disable,
+  /// Check domains with inference for private modules.
+  Infer,
+  /// Check domains without inference.
+  Check,
+  /// Check domains with inference for both public and private modules.
+  InferAll,
+};
+
+/// Convert the "domain mode" firtool option to a "firrtl::InferDomainsMode",
+/// the configuration for a pass.
+constexpr std::optional<firrtl::InferDomainsMode>
+toInferDomainsPassMode(DomainMode mode) {
+  switch (mode) {
+  case DomainMode::Disable:
+    return std::nullopt;
+  case DomainMode::Infer:
+    return firrtl::InferDomainsMode::Infer;
+  case DomainMode::Check:
+    return firrtl::InferDomainsMode::Check;
+  case DomainMode::InferAll:
+    return firrtl::InferDomainsMode::InferAll;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // FirtoolOptions
 //===----------------------------------------------------------------------===//
@@ -144,7 +172,7 @@ class FirtoolOptions {
 
   bool getEmitAllBindFiles() const { return emitAllBindFiles; }
 
-  bool shouldInferDomains() const { return inferDomains; }
+  DomainMode getDomainMode() const { return domainMode; }
 
   // Setters, used by the CAPI
   FirtoolOptions &setOutputFilename(StringRef name) {
@@ -395,8 +423,8 @@ class FirtoolOptions {
     return *this;
   }
 
-  FirtoolOptions &setInferDomains(bool value) {
-    inferDomains = value;
+  FirtoolOptions &setdomainMode(DomainMode value) {
+    domainMode = value;
     return *this;
   }
 
@@ -454,7 +482,7 @@ class FirtoolOptions {
   bool lintStaticAsserts;
   bool lintXmrsInDesign;
   bool emitAllBindFiles;
-  bool inferDomains;
+  DomainMode domainMode;
 };
 
 void registerFirtoolCLOptions();

include/circt/Support/InstanceGraph.h

@@ -59,11 +59,6 @@ class InstanceGraphNode;
 class InstanceRecord
     : public llvm::ilist_node_with_parent<InstanceRecord, InstanceGraphNode> {
 public:
-  /// Get the op that this is tracking.
-  Operation *getOperation() {
-    return instance.getOperation();
-  }
-
   /// Get the instance-like op that this is tracking.
   template <typename TTarget = InstanceOpInterface>
   auto getInstance() {
@@ -118,8 +113,6 @@ class InstanceGraphNode : public llvm::ilist_node<InstanceGraphNode> {
 public:
   InstanceGraphNode() : module(nullptr) {}
 
-  Operation *getOperation() { return module.getOperation(); }
-
   /// Get the module that this node is tracking.
   template <typename TTarget = ModuleOpInterface>
   auto getModule() {

lib/Dialect/FIRRTL/Transforms/InferDomains.cpp

@@ -45,6 +45,16 @@ using InstanceIterator = InstanceGraphNode::UseIterator;
 using InstanceRange = llvm::iterator_range<InstanceIterator>;
 using PortInsertions = SmallVector<std::pair<unsigned, PortInfo>>;
 
+//====--------------------------------------------------------------------------
+// Domain Inference mode helper.
+//====--------------------------------------------------------------------------
+
+template <typename T>
+static bool shouldInfer(T op, InferDomainsMode mode) {
+  return op.isPublic() ? shouldInferPublicModules(mode)
+                       : shouldInferPrivateModules(mode);
+}
+
 //====--------------------------------------------------------------------------
 // Helpers for working with module or instance domain info.
 //====--------------------------------------------------------------------------
@@ -1375,12 +1385,12 @@ static LogicalResult inferModuleDomains(GlobalState &globals,
 // InferDomainsPass: Top-level pass implementation.
 //===---------------------------------------------------------------------------
 
-static LogicalResult runOnModuleLike(bool inferPublic, GlobalState &globals,
-                                     Operation *op) {
+static LogicalResult runOnModuleLike(InferDomainsMode mode,
+                                     GlobalState &globals, Operation *op) {
   if (auto module = dyn_cast<FModuleOp>(op)) {
-    if (module.isPublic() && !inferPublic)
-      return checkModuleDomains(globals, module);
-    return inferModuleDomains(globals, module);
+    if (shouldInfer(module, mode))
+      return inferModuleDomains(globals, module);
+    return checkModuleDomains(globals, module);
   }
 
   if (auto extModule = dyn_cast<FExtModuleOp>(op)) {
@@ -1406,7 +1416,7 @@ void InferDomainsPass::runOnOperation() {
   DenseSet<InstanceGraphNode *> visited;
   for (auto *root : instanceGraph) {
     for (auto *node : llvm::post_order_ext(root, visited)) {
-      if (failed(runOnModuleLike(inferPublic, globals, node->getModule()))) {
+      if (failed(runOnModuleLike(mode, globals, node->getModule()))) {
         signalPassFailure();
         return;
       }

lib/Firtool/Firtool.cpp

@@ -49,9 +49,9 @@ LogicalResult firtool::populatePreprocessTransforms(mlir::PassManager &pm,
   pm.nest<firrtl::CircuitOp>().nest<firrtl::FModuleOp>().addPass(
       firrtl::createLowerIntrinsics());
 
-  if (opt.shouldInferDomains())
-    pm.nest<firrtl::CircuitOp>().addPass(firrtl::createInferDomains());
-
+  if (auto mode = toInferDomainsPassMode(opt.getDomainMode())) {
+    pm.nest<firrtl::CircuitOp>().addPass(firrtl::createInferDomains({*mode}));
+  }
   return success();
 }
 
@@ -761,10 +761,19 @@ struct FirtoolCmdOptions {
       llvm::cl::desc("Emit bindfiles for private modules"),
       llvm::cl::init(false)};
 
-    llvm::cl::opt<bool> inferDomains{
-      "infer-domains",
-      llvm::cl::desc("Enable domain inference and checking"),
-      llvm::cl::init(false)};
+  llvm::cl::opt<firtool::DomainMode> domainMode{
+      "domain-mode", llvm::cl::desc("Enable domain inference and checking"),
+      llvm::cl::init(firtool::DomainMode::Disable),
+      llvm::cl::values(
+          clEnumValN(firtool::DomainMode::Disable, "disable",
+                     "Disable domain checking"),
+          clEnumValN(firtool::DomainMode::Infer, "infer",
+                     "Check domains with inference for private modules"),
+          clEnumValN(firtool::DomainMode::Check, "check",
+                     "Check domains without inference"),
+          clEnumValN(firtool::DomainMode::InferAll, "infer-all",
+                     "Check domains with inference for both public and private "
+                     "modules"))};
 
   //===----------------------------------------------------------------------===
   // Lint options
@@ -817,7 +826,8 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
       disableCSEinClasses(false), selectDefaultInstanceChoice(false),
       symbolicValueLowering(verif::SymbolicValueLowering::ExtModule),
       disableWireElimination(false), lintStaticAsserts(true),
-      lintXmrsInDesign(true), emitAllBindFiles(false), inferDomains(false) {
+      lintXmrsInDesign(true), emitAllBindFiles(false),
+      domainMode(DomainMode::Disable) {
   if (!clOptions.isConstructed())
     return;
   outputFilename = clOptions->outputFilename;
@@ -870,5 +880,5 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
   lintStaticAsserts = clOptions->lintStaticAsserts;
   lintXmrsInDesign = clOptions->lintXmrsInDesign;
   emitAllBindFiles = clOptions->emitAllBindFiles;
-  inferDomains = clOptions->inferDomains;
+  domainMode = clOptions->domainMode;
 }

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

@@ -1,4 +1,4 @@
-// RUN: circt-opt -pass-pipeline='builtin.module(firrtl.circuit(firrtl-infer-domains{infer-public=true}))' %s --verify-diagnostics --split-input-file
+// RUN: circt-opt -pass-pipeline='builtin.module(firrtl.circuit(firrtl-infer-domains{mode=infer-all}))' %s --verify-diagnostics --split-input-file
 
 // Port annotated with same domain type twice.
 firrtl.circuit "DomainCrossOnPort" {
@@ -112,18 +112,38 @@ firrtl.circuit "Top" {
 
 // -----
 
-// Domain not exported like it should be.
+// Domain exported multiple times. Which do we choose?
+
+firrtl.circuit "DoubleExportOfDomain" {
+    firrtl.domain @ClockDomain
+
+  firrtl.module @DoubleExportOfDomain(
+    // expected-note @below {{candidate association "DI"}}
+    in  %DI : !firrtl.domain of @ClockDomain,
+    // 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"}}
+    out %o  : !firrtl.uint<1> domains []
+  ) {
+    // DI and DO are aliases
+    firrtl.domain.define %DO, %DI
+
+    // o is on same domain as i
+    firrtl.matchingconnect %o, %i : !firrtl.uint<1>
+  }
+}
 
 // -----
 
-// Domain exported multiple times. Which do we choose?
+// Domain exported multiple times, this time with one input and one output. Which do we choose?
 
 firrtl.circuit "DoubleExportOfDomain" {
-  firrtl.domain @ClockDomain
+    firrtl.domain @ClockDomain
 
   firrtl.module @DoubleExportOfDomain(
     // expected-note @below {{candidate association "DI"}}
-    in  %DI : !firrtl.domain of @ClockDomain,
+    out  %DI : !firrtl.domain of @ClockDomain,
     // expected-note @below {{candidate association "DO"}}
     out %DO : !firrtl.domain of @ClockDomain,
     in  %i  : !firrtl.uint<1> domains [%DO],
@@ -138,3 +158,31 @@ firrtl.circuit "DoubleExportOfDomain" {
   }
 }
 
+// -----
+
+// InstanceChoice: Each module has different domains inferred.
+
+firrtl.circuit "ConflictingInstanceChoiceDomains" {
+    firrtl.domain @ClockDomain
+
+  firrtl.option @Option {
+    firrtl.option_case @X
+    firrtl.option_case @Y
+  }
+
+  // Foo's "out" port takes on the domains of "in1".
+  firrtl.module @Foo(in %in1: !firrtl.uint<1>, in %in2: !firrtl.uint<1>, out %out: !firrtl.uint<1>) {
+    firrtl.connect %out, %in1 : !firrtl.uint<1>
+  }
+
+  // Bar's "out" port takes on the domains of "in2".
+  firrtl.module @Bar(in %in1: !firrtl.uint<1>, in %in2: !firrtl.uint<1>, out %out: !firrtl.uint<1>) {
+    firrtl.connect %out, %in2 : !firrtl.uint<1>
+  }
+
+  firrtl.module @ConflictingInstanceChoiceDomains(in %in1: !firrtl.uint<1>, in %in2: !firrtl.uint<1>) {
+    %inst_in1, %inst_in2, %inst_out = firrtl.instance_choice inst @Foo alternatives @Option { @X -> @Foo, @Y -> @Bar } (in in1: !firrtl.uint<1>, in in2: !firrtl.uint<1>, out out: !firrtl.uint<1>)
+    firrtl.connect %inst_in1, %in1 : !firrtl.uint<1>, !firrtl.uint<1>
+    firrtl.connect %inst_in2, %in2 : !firrtl.uint<1>, !firrtl.uint<1>
+  }
+}