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()