From 002427065e8b0751a5c0ed8a980addf99f7ad82a Mon Sep 17 00:00:00 2001 From: Mike Urbach Date: Fri, 20 Sep 2024 19:50:55 -0600 Subject: [PATCH] [FIRRTL] Allow local targets to be multiply-instantiated. (#7613) The existing path support was built up based on the assumption that every target is unique. That is true for FIRRTL produced by standard Chisel code, which elaborates unique modules for each instance. We definitely don't want to limit ourselves to this world, and we should support targeting things that are multiply instantiated when it is not ambiguous what we refer to. This patch relaxes the single-instantiation constraints for local targets, which refer to a module or something inside a module, regardless of how many times or at what paths that particular module was instantiated. This required a couple changes through the pipeline: ResolvePaths already had an early exit for the local path case, but this needed to come before the single-instantiation check. LowerClasses needed a couple small changes to not enforce the single-instantiation check in the local path case, and to build a hierpath that just has a single element. While this is not a new requirement, we can still get ambiguous local targets, for instance from nested module prefixing. The error message in LowerClasses for this case was made a little more clear. --- .../FIRRTL/Transforms/LowerClasses.cpp | 26 ++++++++++++------- .../FIRRTL/Transforms/ResolvePaths.cpp | 11 ++++---- test/Dialect/FIRRTL/lower-classes-errors.mlir | 4 +-- test/Dialect/FIRRTL/lower-classes.mlir | 25 +++++++++++++++++- test/Dialect/FIRRTL/resolve-paths-errors.mlir | 12 +++++---- test/Dialect/FIRRTL/resolve-paths.mlir | 24 +++++++++++++++++ 6 files changed, 79 insertions(+), 23 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp index d77f52c73972..398949c55605 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp @@ -275,7 +275,8 @@ struct PathTracker { // and `owningModule`. FailureOr getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName, - FModuleOp owningModule); + FModuleOp owningModule, + bool isNonLocal); FModuleLike module; // Local data structures. @@ -364,7 +365,8 @@ LogicalResult PathTracker::runOnModule() { FailureOr PathTracker::getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName, - FModuleOp owningModule) { + FModuleOp owningModule, + bool isNonLocal) { auto it = needsAltBasePathCache.find({moduleName, owningModule}); if (it != needsAltBasePathCache.end()) @@ -383,9 +385,9 @@ PathTracker::getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName, needsAltBasePath = true; break; } - // If there is more than one instance of this module, then the path - // operation is ambiguous, which is an error. - if (!node->hasOneUse()) { + // If there is more than one instance of this module, and the target is + // non-local, then the path operation is ambiguous, which is an error. + if (isNonLocal && !node->hasOneUse()) { auto diag = mlir::emitError(loc) << "unable to uniquely resolve target due " "to multiple instantiation"; @@ -517,8 +519,8 @@ PathTracker::processPathTrackers(const AnnoTarget &target) { } // Check if we need an alternative base path. - auto needsAltBasePath = - getOrComputeNeedsAltBasePath(op->getLoc(), moduleName, owningModule); + auto needsAltBasePath = getOrComputeNeedsAltBasePath( + op->getLoc(), moduleName, owningModule, hierPathOp); if (failed(needsAltBasePath)) { error = true; return false; @@ -528,6 +530,10 @@ PathTracker::processPathTrackers(const AnnoTarget &target) { // to the start of the annotation's NLA. InstanceGraphNode *node = instanceGraph.lookup(moduleName); while (true) { + // If it's not a non-local target, we don't have to append anything. + if (!hierPathOp) + break; + // If we get to the owning module or the top, we're done. if (node->getModule() == owningModule || node->noUses()) break; @@ -582,8 +588,10 @@ LogicalResult PathTracker::updatePathInfoTable(PathInfoTable &pathInfoTable, auto &pathInfo = it->second; if (!inserted) { assert(pathInfo.loc.has_value() && "all PathInfo should have a Location"); - auto diag = emitError(pathInfo.loc.value(), "duplicate identifier found"); - diag.attachNote(entry.op->getLoc()) << "other identifier here"; + auto diag = emitError(pathInfo.loc.value(), + "path identifier already found, paths must resolve " + "to a unique target"); + diag.attachNote(entry.op->getLoc()) << "other path identifier here"; return failure(); } diff --git a/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp b/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp index 2ed6c01f9827..d8795b37e749 100644 --- a/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp +++ b/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp @@ -41,6 +41,11 @@ struct PathResolver { const AnnoPathValue &target, FlatSymbolRefAttr &result) { + // If the path is empty then this is a local reference and we should not + // construct a HierPathOp. + if (target.instances.empty()) + return success(); + // We want to root this path at the top level module, or in the case of an // unreachable module, we settle for as high as we can get. auto module = target.ref.getModule(); @@ -69,12 +74,6 @@ struct PathResolver { node = (*node->usesBegin())->getParent(); } - // If the path is empty then this is a local reference and we should not - // construct a HierPathOp. - if (target.instances.empty()) { - return success(); - } - // Transform the instances into a list of FlatSymbolRefs. SmallVector insts; insts.reserve(target.instances.size()); diff --git a/test/Dialect/FIRRTL/lower-classes-errors.mlir b/test/Dialect/FIRRTL/lower-classes-errors.mlir index c264e649e11d..176c82605a2a 100644 --- a/test/Dialect/FIRRTL/lower-classes-errors.mlir +++ b/test/Dialect/FIRRTL/lower-classes-errors.mlir @@ -32,9 +32,9 @@ firrtl.circuit "PathIllegalHierpath" { firrtl.circuit "PathDuplicateID" { firrtl.module @PathDuplicateID() { - // expected-error @below {{duplicate identifier found}} + // expected-error @below {{path identifier already found, paths must resolve to a unique target}} %a = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} : !firrtl.uint<8> - // expected-note @below {{other identifier here}} + // expected-note @below {{other path identifier here}} %b = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} : !firrtl.uint<8> } } diff --git a/test/Dialect/FIRRTL/lower-classes.mlir b/test/Dialect/FIRRTL/lower-classes.mlir index 5ab1befacc50..02fcd5613822 100644 --- a/test/Dialect/FIRRTL/lower-classes.mlir +++ b/test/Dialect/FIRRTL/lower-classes.mlir @@ -158,6 +158,7 @@ firrtl.circuit "PathModule" { // CHECK: hw.hierpath private [[WIRE_PATH:@.+]] [@PathModule::[[WIRE_SYM:@.+]]] // CHECK: hw.hierpath private [[VECTOR_PATH:@.+]] [@PathModule::[[VECTOR_SYM:@.+]]] // CHECK: hw.hierpath private [[INST_PATH:@.+]] [@PathModule::@child] + // CHECK: hw.hierpath private [[LOCAL_PATH:@.+]] [@Child] // CHECK: hw.hierpath private [[MODULE_PATH:@.+]] [@PathModule::@child, @Child::[[NONLOCAL_SYM:@.+]]] // CHECK: firrtl.module @PathModule(in %in: !firrtl.uint<1> sym [[PORT_SYM]]) { @@ -209,7 +210,7 @@ firrtl.circuit "PathModule" { // CHECK: om.path_create member_instance %basepath [[INST_PATH]] // CHECK: om.path_create member_instance %basepath [[INST_PATH]] // CHECK: om.path_create instance %basepath [[INST_PATH]] - // CHECK: om.path_create reference %basepath [[NONLOCAL_PATH]] + // CHECK: om.path_create reference %basepath [[LOCAL_PATH]] %instance_member_instance = firrtl.path member_instance distinct[4]<> %instance_member_reference = firrtl.path member_reference distinct[4]<> %instance = firrtl.path instance distinct[4]<> @@ -471,3 +472,25 @@ firrtl.circuit "PathTargetReplaced" { firrtl.module private @WillBeReplaced(out %output: !firrtl.integer) { } } + +// CHECK-LABEL: firrtl.circuit "LocalPath" +firrtl.circuit "LocalPath" { + // CHECK: hw.hierpath private [[MODULE_NLA:@.+]] [@Child] + // CHECK: hw.hierpath private [[WIRE_NLA:@.+]] [@Child::[[WIRE_SYM:@.+]]] + + firrtl.module @Child() attributes {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} { + // CHECK: firrtl.wire sym [[WIRE_SYM]] + %wire = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[1]<>}]} : !firrtl.uint<8> + } + + firrtl.module @LocalPath() { + // CHECK: om.path_create instance %basepath [[MODULE_NLA]] + %0 = firrtl.path instance distinct[0]<> + + // CHECK: om.path_create reference %basepath [[WIRE_NLA]] + %1 = firrtl.path reference distinct[1]<> + + firrtl.instance child0 @Child() + firrtl.instance child1 @Child() + } +} diff --git a/test/Dialect/FIRRTL/resolve-paths-errors.mlir b/test/Dialect/FIRRTL/resolve-paths-errors.mlir index d8bf3c3e3b4e..82d4eaed1f47 100644 --- a/test/Dialect/FIRRTL/resolve-paths-errors.mlir +++ b/test/Dialect/FIRRTL/resolve-paths-errors.mlir @@ -75,13 +75,16 @@ firrtl.module @VectorTarget(in %a : !firrtl.vector, 1>) { firrtl.circuit "AmbiguousPath" { firrtl.module @AmbiguousPath() { // expected-error @below {{unable to uniquely resolve target due to multiple instantiation}} - %0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousPath|Child" + %0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousPath|Child/grandchild:GrandChild" // expected-note @below {{instance here}} firrtl.instance child0 @Child() // expected-note @below {{instance here}} firrtl.instance child1 @Child() } -firrtl.module @Child() {} +firrtl.module @Child() { + firrtl.instance grandchild @GrandChild() +} +firrtl.module @GrandChild() {} } // ----- @@ -102,18 +105,17 @@ firrtl.class @Test(){ firrtl.circuit "UpwardPath" { firrtl.module @UpwardPath() { - %wire = firrtl.wire : !firrtl.uint<8> firrtl.instance child @Child() } firrtl.module @Child() { %om = firrtl.object @OM() - + %wire = firrtl.wire : !firrtl.uint<8> } firrtl.class @OM(){ // expected-error @below {{unable to resolve path relative to owning module "Child"}} - %0 = firrtl.unresolved_path "OMReferenceTarget:~UpwardPath|UpwardPath>wire" + %0 = firrtl.unresolved_path "OMReferenceTarget:~UpwardPath|UpwardPath/child:Child>wire" } } diff --git a/test/Dialect/FIRRTL/resolve-paths.mlir b/test/Dialect/FIRRTL/resolve-paths.mlir index 80d97288f3d4..d94436f26aa8 100644 --- a/test/Dialect/FIRRTL/resolve-paths.mlir +++ b/test/Dialect/FIRRTL/resolve-paths.mlir @@ -91,6 +91,30 @@ firrtl.module @Child() { // ----- +// CHECK-LABEL: firrtl.circuit "LocalPath" +firrtl.circuit "LocalPath" { +// CHECK: firrtl.module @LocalPath() +firrtl.module @LocalPath() { + // CHECK: %0 = firrtl.path instance distinct[0]<> + %0 = firrtl.unresolved_path "OMInstanceTarget:~LocalPath|Child" + + // CHECK: %1 = firrtl.path reference distinct[1]<> + %1 = firrtl.unresolved_path "OMReferenceTarget:~LocalPath|Child>wire" + + // CHECK: firrtl.instance child0 @Child() + // CHECK: firrtl.instance child1 @Child() + firrtl.instance child0 @Child() + firrtl.instance child1 @Child() +} +// CHECK: firrtl.module @Child() attributes {annotations = [{class = "circt.tracker", id = distinct[0]<>}]} +firrtl.module @Child() { + // CHECK: %wire = firrtl.wire {annotations = [{class = "circt.tracker", id = distinct[1]<>}]} : !firrtl.uint<8> + %wire = firrtl.wire : !firrtl.uint<8> +} +} + +// ----- + // CHECK-LABEL: firrtl.circuit "PathMinimization" firrtl.circuit "PathMinimization" { // CHECK: firrtl.module @PathMinimization()