[FIRRTL] Error if asked to add a port to a public module.

include/circt-c/Firtool/Firtool.h

@@ -85,6 +85,9 @@ MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetDisableAnnotationsClassless(
 MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetLowerAnnotationsNoRefTypePorts(
     CirctFirtoolFirtoolOptions options, bool value);
 
+MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetAllowAddingPortsOnPublic(
+    CirctFirtoolFirtoolOptions options, bool value);
+
 MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetPreserveAggregate(
     CirctFirtoolFirtoolOptions options,
     CirctFirtoolPreserveAggregateMode value);

include/circt/Dialect/FIRRTL/Passes.h

@@ -30,7 +30,8 @@ std::unique_ptr<mlir::Pass> createResolvePathsPass();
 std::unique_ptr<mlir::Pass>
 createLowerFIRRTLAnnotationsPass(bool ignoreUnhandledAnnotations = false,
                                  bool ignoreClasslessAnnotations = false,
-                                 bool noRefTypePorts = false);
+                                 bool noRefTypePorts = false,
+                                 bool allowAddingPortsOnPublic = false);
 
 std::unique_ptr<mlir::Pass> createLowerOpenAggsPass();
 

include/circt/Dialect/FIRRTL/Passes.td

@@ -41,6 +41,8 @@ def LowerFIRRTLAnnotations : Pass<"firrtl-lower-annotations", "firrtl::CircuitOp
       "Ignore unknown annotations.">,
     Option<"noRefTypePorts", "no-ref-type-ports", "bool", "false",
       "Create normal ports, not ref type ports.">,
+    Option<"allowAddingPortsOnPublic", "allow-adding-ports-on-public-modules", "bool", "false",
+      "Allow public modules to gain additional ports as a result of wiring.">,
   ];
   let dependentDialects = ["hw::HWDialect"];
   let statistics = [
@@ -54,6 +56,8 @@ def LowerFIRRTLAnnotations : Pass<"firrtl-lower-annotations", "firrtl::CircuitOp
       "Number of unhandled annotations">,
     Statistic<"numReusedHierPathOps", "num-reused-hierpath",
       "Number of reused HierPathOp's">,
+    Statistic<"numPublicPortsWired", "num-public-ports-wired",
+      "Number of ports on public modules added due to wiring">,
   ];
 }
 

include/circt/Firtool/Firtool.h

@@ -92,6 +92,9 @@ class FirtoolOptions {
   bool shouldLowerNoRefTypePortAnnotations() const {
     return lowerAnnotationsNoRefTypePorts;
   }
+  bool shouldAllowAddingPortsOnPublic() const {
+    return allowAddingPortsOnPublic;
+  }
   bool shouldReplicateSequentialMemories() const { return replSeqMem; }
   bool shouldDisableOptimization() const { return disableOptimization; }
   bool shouldLowerMemories() const { return lowerMemories; }
@@ -153,6 +156,11 @@ class FirtoolOptions {
     return *this;
   }
 
+  FirtoolOptions &setAllowAddingPortsOnPublic(bool value) {
+    allowAddingPortsOnPublic = value;
+    return *this;
+  }
+
   FirtoolOptions &
   setPreserveAggregate(firrtl::PreserveAggregate::PreserveMode value) {
     preserveAggregate = value;
@@ -361,6 +369,7 @@ class FirtoolOptions {
   bool disableAnnotationsUnknown;
   bool disableAnnotationsClassless;
   bool lowerAnnotationsNoRefTypePorts;
+  bool allowAddingPortsOnPublic;
   firrtl::PreserveAggregate::PreserveMode preserveAggregate;
   firrtl::PreserveValues::PreserveMode preserveMode;
   bool enableDebugInfo;

lib/CAPI/Firtool/Firtool.cpp

@@ -51,6 +51,11 @@ void circtFirtoolOptionsSetLowerAnnotationsNoRefTypePorts(
   unwrap(options)->setLowerAnnotationsNoRefTypePorts(value);
 }
 
+void circtFirtoolOptionsSetAllowAddingPortsOnPublic(
+    CirctFirtoolFirtoolOptions options, bool value) {
+  unwrap(options)->setAllowAddingPortsOnPublic(value);
+}
+
 void circtFirtoolOptionsSetPreserveAggregate(
     CirctFirtoolFirtoolOptions options,
     CirctFirtoolPreserveAggregateMode value) {

lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp

@@ -545,6 +545,7 @@ struct LowerAnnotationsPass
   LogicalResult legacyToWiringProblems(ApplyState &state);
   LogicalResult solveWiringProblems(ApplyState &state);
 
+  using LowerFIRRTLAnnotationsBase::allowAddingPortsOnPublic;
   using LowerFIRRTLAnnotationsBase::ignoreAnnotationClassless;
   using LowerFIRRTLAnnotationsBase::ignoreAnnotationUnknown;
   using LowerFIRRTLAnnotationsBase::noRefTypePorts;
@@ -874,11 +875,21 @@ LogicalResult LowerAnnotationsPass::solveWiringProblems(ApplyState &state) {
 
     // Record module modifications related to adding ports to modules.
     auto addPorts = [&](igraph::InstancePath insts, Value val, Type tpe,
-                        Direction dir) {
+                        Direction dir) -> LogicalResult {
       StringRef name, instName;
       for (auto instNode : llvm::reverse(insts)) {
         auto inst = cast<InstanceOp>(*instNode);
         auto mod = inst.getReferencedModule<FModuleOp>(instanceGraph);
+        if (mod.isPublic()) {
+          if (!allowAddingPortsOnPublic) {
+            auto diag = emitError(
+                mod.getLoc(), "cannot wire port through this public module");
+            diag.attachNote(source.getLoc()) << "source here";
+            diag.attachNote(sink.getLoc()) << "sink here";
+            return diag;
+          }
+          ++numPublicPortsWired;
+        }
         if (name.empty()) {
           if (problem.newNameHint.empty())
             name = state.getNamespace(mod).newName(
@@ -897,11 +908,13 @@ LogicalResult LowerAnnotationsPass::solveWiringProblems(ApplyState &state) {
             {index, {StringAttr::get(context, name), tpe, dir}});
         instName = inst.getInstanceName();
       }
+      return success();
     };
 
     // Record the addition of ports.
-    addPorts(sources, source, sourceType, Direction::Out);
-    addPorts(sinks, sink, sinkType, Direction::In);
+    if (failed(addPorts(sources, source, sourceType, Direction::Out)) ||
+        failed(addPorts(sinks, sink, sinkType, Direction::In)))
+      return failure();
   }
 
   // Iterate over modules from leaves to roots, applying ModuleModifications to
@@ -1053,13 +1066,13 @@ void LowerAnnotationsPass::runOnOperation() {
 }
 
 /// This is the pass constructor.
-std::unique_ptr<mlir::Pass>
-circt::firrtl::createLowerFIRRTLAnnotationsPass(bool ignoreAnnotationUnknown,
-                                                bool ignoreAnnotationClassless,
-                                                bool noRefTypePorts) {
+std::unique_ptr<mlir::Pass> circt::firrtl::createLowerFIRRTLAnnotationsPass(
+    bool ignoreAnnotationUnknown, bool ignoreAnnotationClassless,
+    bool noRefTypePorts, bool allowAddingPortsOnPublic) {
   auto pass = std::make_unique<LowerAnnotationsPass>();
   pass->ignoreAnnotationUnknown = ignoreAnnotationUnknown;
   pass->ignoreAnnotationClassless = ignoreAnnotationClassless;
   pass->noRefTypePorts = noRefTypePorts;
+  pass->allowAddingPortsOnPublic = allowAddingPortsOnPublic;
   return pass;
 }

lib/Firtool/Firtool.cpp

@@ -33,7 +33,8 @@ LogicalResult firtool::populatePreprocessTransforms(mlir::PassManager &pm,
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerFIRRTLAnnotationsPass(
       opt.shouldDisableUnknownAnnotations(),
       opt.shouldDisableClasslessAnnotations(),
-      opt.shouldLowerNoRefTypePortAnnotations()));
+      opt.shouldLowerNoRefTypePortAnnotations(),
+      opt.shouldAllowAddingPortsOnPublic()));
 
   if (opt.shouldEnableDebugInfo())
     pm.nest<firrtl::CircuitOp>().addNestedPass<firrtl::FModuleOp>(
@@ -422,6 +423,11 @@ struct FirtoolCmdOptions {
           "wiring problems inside the LowerAnnotations pass"),
       llvm::cl::init(false), llvm::cl::Hidden};
 
+  llvm::cl::opt<bool> allowAddingPortsOnPublic{
+      "allow-adding-ports-on-public-modules",
+      llvm::cl::desc("Allow adding ports to public modules"),
+      llvm::cl::init(false), llvm::cl::Hidden};
+
   llvm::cl::opt<circt::firrtl::PreserveAggregate::PreserveMode>
       preserveAggregate{
           "preserve-aggregate",
@@ -703,6 +709,7 @@ void circt::firtool::registerFirtoolCLOptions() {
 circt::firtool::FirtoolOptions::FirtoolOptions()
     : outputFilename("-"), disableAnnotationsUnknown(false),
       disableAnnotationsClassless(false), lowerAnnotationsNoRefTypePorts(false),
+      allowAddingPortsOnPublic(false),
       preserveAggregate(firrtl::PreserveAggregate::None),
       preserveMode(firrtl::PreserveValues::None), enableDebugInfo(false),
       buildMode(BuildModeRelease), disableOptimization(false),
@@ -729,6 +736,7 @@ circt::firtool::FirtoolOptions::FirtoolOptions()
   disableAnnotationsUnknown = clOptions->disableAnnotationsUnknown;
   disableAnnotationsClassless = clOptions->disableAnnotationsClassless;
   lowerAnnotationsNoRefTypePorts = clOptions->lowerAnnotationsNoRefTypePorts;
+  allowAddingPortsOnPublic = clOptions->allowAddingPortsOnPublic;
   preserveAggregate = clOptions->preserveAggregate;
   preserveMode = clOptions->preserveMode;
   enableDebugInfo = clOptions->enableDebugInfo;

test/Dialect/FIRRTL/SFCTests/mem-taps-reg.fir

@@ -1,4 +1,4 @@
-; RUN: firtool --verilog %s | FileCheck %s
+; RUN: firtool --verilog --allow-adding-ports-on-public-modules %s | FileCheck %s
 
 circuit Top : %[[
   {

test/Dialect/FIRRTL/SFCTests/mem-taps.fir

@@ -1,6 +1,6 @@
-; RUN: firtool --verilog %s | FileCheck %s
-; RUN: firtool --verilog -preserve-aggregate=1d-vec %s | FileCheck %s --check-prefix=AGGGREGATE
-; RUN: firtool --verilog -lower-annotations-no-ref-type-ports %s | FileCheck %s --check-prefix=NOREFS
+; RUN: firtool --verilog -allow-adding-ports-on-public-modules %s | FileCheck %s
+; RUN: firtool --verilog -allow-adding-ports-on-public-modules -preserve-aggregate=1d-vec %s | FileCheck %s --check-prefix=AGGGREGATE
+; RUN: firtool --verilog -allow-adding-ports-on-public-modules -lower-annotations-no-ref-type-ports %s | FileCheck %s --check-prefix=NOREFS
 
 circuit Top : %[[
   {

test/Dialect/FIRRTL/annotations.mlir

@@ -1,4 +1,4 @@
-// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl-lower-annotations))' --split-input-file %s | FileCheck %s
+// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl-lower-annotations{allow-adding-ports-on-public-modules=true}))' --split-input-file %s | FileCheck %s
 
 // circt.test copies the annotation to the target
 // circt.testNT puts the targetless annotation on the circuit

test/Dialect/FIRRTL/legacy-wiring-errors.mlir

@@ -90,3 +90,34 @@ firrtl.circuit "Foo" attributes {
     firrtl.strictconnect %x, %invalid_ui1 : !firrtl.uint<1>
   }
 }
+
+// -----
+
+// Error on wiring through public
+firrtl.circuit "FooBar" attributes {
+  rawAnnotations = [
+    {
+      class = "firrtl.passes.wiring.SinkAnnotation",
+      target = "FooBar.Foo.out",
+      pin = "foo_out"
+    },
+    {
+      class = "firrtl.passes.wiring.SourceAnnotation",
+      target = "FooBar.FooBar.io.in",
+      pin = "foo_out"
+    }]} {
+  // expected-note @below {{sink here}}
+  firrtl.module private @Foo(out %out: !firrtl.uint<1>) {
+  }
+  // expected-error @below {{cannot wire port through this public module}}
+  firrtl.module public @Bar(out %out: !firrtl.uint<1>) {
+      %foo_out = firrtl.instance foo interesting_name  @Foo(out out: !firrtl.uint<1>)
+      firrtl.strictconnect %out, %foo_out : !firrtl.uint<1>
+  }
+  // expected-note @below {{source here}}
+  firrtl.module @FooBar(out %io: !firrtl.bundle<in flip: uint<1>, out: uint<1>>) {
+      %0 = firrtl.subfield %io[out] : !firrtl.bundle<in flip: uint<1>, out: uint<1>>
+      %bar_out = firrtl.instance bar interesting_name  @Bar(out out: !firrtl.uint<1>)
+      firrtl.strictconnect %0, %bar_out : !firrtl.uint<1>
+  }
+}

test/Dialect/FIRRTL/legacy-wiring.mlir

@@ -14,16 +14,16 @@ firrtl.circuit "FooBar" attributes {
       target = "FooBar.FooBar.io.in",
       pin = "foo_out"
     }]} {
-  // CHECK: firrtl.module @Foo
+  // CHECK: firrtl.module private @Foo
   // The real port type of the source should be bored
   // CHECK-SAME: in %io_out__bore: !firrtl.uint<1>
-  firrtl.module@Foo(out %io: !firrtl.bundle<out: uint<1>>) {
+  firrtl.module private @Foo(out %io: !firrtl.bundle<out: uint<1>>) {
       firrtl.skip
   }
-  // CHECK: firrtl.module @Bar
+  // CHECK: firrtl.module private @Bar
   // The real port type of the source should be bored in the parent
   // CHECK-SAME: in %foo_io_out__bore: !firrtl.uint<1>
-  firrtl.module @Bar(out %io: !firrtl.bundle<out: uint<1>>) {
+  firrtl.module private @Bar(out %io: !firrtl.bundle<out: uint<1>>) {
       %0 = firrtl.subfield %io[out] : !firrtl.bundle<out: uint<1>>
       // CHECK: firrtl.instance foo
       // CHECK-SAME: in io_out__bore: !firrtl.uint<1>
@@ -68,23 +68,23 @@ firrtl.circuit "FooBar" attributes {
       target = "FooBar.Bar.io.out",
       pin = "in"
     }]} {
-  // CHECK: firrtl.module @Foo
+  // CHECK: firrtl.module private @Foo
   // CHECK-SAME: in %io_out__bore: !firrtl.uint<1>
-  firrtl.module @Foo(out %io: !firrtl.bundle<out: uint<1>>) {
+  firrtl.module private @Foo(out %io: !firrtl.bundle<out: uint<1>>) {
     firrtl.skip
     // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle<out: uint<1>>
     // CHECK: firrtl.strictconnect %0, %io_out__bore : !firrtl.uint<1>
   }
-  // CHECK: firrtl.module @Foo_1
+  // CHECK: firrtl.module private @Foo_1
   // CHECK-SAME: in %io_out__bore: !firrtl.uint<1>
-  firrtl.module @Foo_1(out %io: !firrtl.bundle<out: uint<1>>) {
+  firrtl.module private @Foo_1(out %io: !firrtl.bundle<out: uint<1>>) {
     firrtl.skip
     // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle<out: uint<1>>
     // CHECK: firrtl.strictconnect %0, %io_out__bore : !firrtl.uint<1>
   }
-  // CHECK: firrtl.module @Bar
+  // CHECK: firrtl.module private @Bar
   // CHECK-SAME: in %io_out__bore: !firrtl.uint<1>
-  firrtl.module @Bar(out %io: !firrtl.bundle<out: uint<1>>) {
+  firrtl.module private @Bar(out %io: !firrtl.bundle<out: uint<1>>) {
     firrtl.skip
     // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle<out: uint<1>>
     // CHECK: firrtl.strictconnect %0, %io_out__bore : !firrtl.uint<1>