diff --git a/include/circt-c/Firtool/Firtool.h b/include/circt-c/Firtool/Firtool.h index 162d9cef40ae..98740885a73d 100644 --- a/include/circt-c/Firtool/Firtool.h +++ b/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); diff --git a/include/circt/Dialect/FIRRTL/Passes.h b/include/circt/Dialect/FIRRTL/Passes.h index 3938445f166b..82b4154aa380 100644 --- a/include/circt/Dialect/FIRRTL/Passes.h +++ b/include/circt/Dialect/FIRRTL/Passes.h @@ -30,7 +30,8 @@ std::unique_ptr createResolvePathsPass(); std::unique_ptr createLowerFIRRTLAnnotationsPass(bool ignoreUnhandledAnnotations = false, bool ignoreClasslessAnnotations = false, - bool noRefTypePorts = false); + bool noRefTypePorts = false, + bool allowAddingPortsOnPublic = false); std::unique_ptr createLowerOpenAggsPass(); diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index afa5578e8889..4193b6abf370 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/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">, ]; } diff --git a/include/circt/Firtool/Firtool.h b/include/circt/Firtool/Firtool.h index 890ee9bac1d0..b4c0088c74cb 100644 --- a/include/circt/Firtool/Firtool.h +++ b/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; diff --git a/lib/CAPI/Firtool/Firtool.cpp b/lib/CAPI/Firtool/Firtool.cpp index 970b248796a6..de60d7ad9752 100644 --- a/lib/CAPI/Firtool/Firtool.cpp +++ b/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) { diff --git a/lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp b/lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp index 225f7bb29480..a390b6993c7a 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerAnnotations.cpp +++ b/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(*instNode); auto mod = inst.getReferencedModule(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 -circt::firrtl::createLowerFIRRTLAnnotationsPass(bool ignoreAnnotationUnknown, - bool ignoreAnnotationClassless, - bool noRefTypePorts) { +std::unique_ptr circt::firrtl::createLowerFIRRTLAnnotationsPass( + bool ignoreAnnotationUnknown, bool ignoreAnnotationClassless, + bool noRefTypePorts, bool allowAddingPortsOnPublic) { auto pass = std::make_unique(); pass->ignoreAnnotationUnknown = ignoreAnnotationUnknown; pass->ignoreAnnotationClassless = ignoreAnnotationClassless; pass->noRefTypePorts = noRefTypePorts; + pass->allowAddingPortsOnPublic = allowAddingPortsOnPublic; return pass; } diff --git a/lib/Firtool/Firtool.cpp b/lib/Firtool/Firtool.cpp index fc4f6fac75f0..062fd2981bdf 100644 --- a/lib/Firtool/Firtool.cpp +++ b/lib/Firtool/Firtool.cpp @@ -33,7 +33,8 @@ LogicalResult firtool::populatePreprocessTransforms(mlir::PassManager &pm, pm.nest().addPass(firrtl::createLowerFIRRTLAnnotationsPass( opt.shouldDisableUnknownAnnotations(), opt.shouldDisableClasslessAnnotations(), - opt.shouldLowerNoRefTypePortAnnotations())); + opt.shouldLowerNoRefTypePortAnnotations(), + opt.shouldAllowAddingPortsOnPublic())); if (opt.shouldEnableDebugInfo()) pm.nest().addNestedPass( @@ -422,6 +423,11 @@ struct FirtoolCmdOptions { "wiring problems inside the LowerAnnotations pass"), llvm::cl::init(false), llvm::cl::Hidden}; + llvm::cl::opt 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 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; diff --git a/test/Dialect/FIRRTL/SFCTests/mem-taps-reg.fir b/test/Dialect/FIRRTL/SFCTests/mem-taps-reg.fir index 2c2adbe43c04..52f5edce4b32 100644 --- a/test/Dialect/FIRRTL/SFCTests/mem-taps-reg.fir +++ b/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 : %[[ { diff --git a/test/Dialect/FIRRTL/SFCTests/mem-taps.fir b/test/Dialect/FIRRTL/SFCTests/mem-taps.fir index 89aebbdebe4b..c305c18488a2 100644 --- a/test/Dialect/FIRRTL/SFCTests/mem-taps.fir +++ b/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 : %[[ { diff --git a/test/Dialect/FIRRTL/annotations.mlir b/test/Dialect/FIRRTL/annotations.mlir index 5bf9736c7166..c636d3d5544d 100644 --- a/test/Dialect/FIRRTL/annotations.mlir +++ b/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 diff --git a/test/Dialect/FIRRTL/legacy-wiring-errors.mlir b/test/Dialect/FIRRTL/legacy-wiring-errors.mlir index db5e2f663bf0..c37690e64a28 100644 --- a/test/Dialect/FIRRTL/legacy-wiring-errors.mlir +++ b/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, out: uint<1>>) { + %0 = firrtl.subfield %io[out] : !firrtl.bundle, out: uint<1>> + %bar_out = firrtl.instance bar interesting_name @Bar(out out: !firrtl.uint<1>) + firrtl.strictconnect %0, %bar_out : !firrtl.uint<1> + } +} diff --git a/test/Dialect/FIRRTL/legacy-wiring.mlir b/test/Dialect/FIRRTL/legacy-wiring.mlir index 2d1932901532..a3609fb3e239 100644 --- a/test/Dialect/FIRRTL/legacy-wiring.mlir +++ b/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>) { + firrtl.module private @Foo(out %io: !firrtl.bundle>) { 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>) { + firrtl.module private @Bar(out %io: !firrtl.bundle>) { %0 = firrtl.subfield %io[out] : !firrtl.bundle> // 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>) { + firrtl.module private @Foo(out %io: !firrtl.bundle>) { firrtl.skip // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle> // 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>) { + firrtl.module private @Foo_1(out %io: !firrtl.bundle>) { firrtl.skip // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle> // 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>) { + firrtl.module private @Bar(out %io: !firrtl.bundle>) { firrtl.skip // CHECK: %0 = firrtl.subfield %io[out] : !firrtl.bundle> // CHECK: firrtl.strictconnect %0, %io_out__bore : !firrtl.uint<1>