diff --git a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp index 1cc16de4f08f..a36b25f170ff 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp @@ -384,16 +384,14 @@ PathTracker::getOrComputeNeedsAltBasePath(Location loc, StringAttr moduleName, break; } // If there is more than one instance of this module, then the path - // operation is ambiguous, which is a warning. This should become an error - // once user code is properly enforcing single instantiation, but in - // practice this generates the same outputs as the original flow for now. - // See https://github.com/llvm/circt/issues/7128. + // operation is ambiguous, which is an error. if (!node->hasOneUse()) { - auto diag = mlir::emitWarning(loc) + auto diag = mlir::emitError(loc) << "unable to uniquely resolve target due " "to multiple instantiation"; for (auto *use : node->uses()) diag.attachNote(use->getInstance().getLoc()) << "instance here"; + return diag; } node = (*node->usesBegin())->getParent(); } @@ -534,10 +532,8 @@ PathTracker::processPathTrackers(const AnnoTarget &target) { if (node->getModule() == owningModule || node->noUses()) break; - // Append the next level of hierarchy to the path. Note that if there - // are multiple instances, which we warn about, this is where the - // ambiguity manifests. In practice, just picking usesBegin generates - // the same output as EmitOMIR would for now. + // Append the next level of hierarchy to the path. + assert(node->hasOneUse() && "expected single instantiation"); InstanceRecord *inst = *node->usesBegin(); path.push_back( OpAnnoTarget(inst->getInstance()) diff --git a/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp b/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp index a98de8492366..2ed6c01f9827 100644 --- a/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp +++ b/lib/Dialect/FIRRTL/Transforms/ResolvePaths.cpp @@ -58,15 +58,13 @@ struct PathResolver { << "unable to resolve path relative to owning module " << owningModule.getModuleNameAttr(); // If there is more than one instance of this module, then the path - // operation is ambiguous, which is a warning. This should become an error - // once user code is properly enforcing single instantiation, but in - // practice this generates the same outputs as the original flow for now. - // See https://github.com/llvm/circt/issues/7128. + // operation is ambiguous, which is an error. if (!node->hasOneUse()) { - auto diag = emitWarning(loc) << "unable to uniquely resolve target due " - "to multiple instantiation"; + auto diag = emitError(loc) << "unable to uniquely resolve target due " + "to multiple instantiation"; for (auto *use : node->uses()) diag.attachNote(use->getInstance().getLoc()) << "instance here"; + return diag; } node = (*node->usesBegin())->getParent(); } diff --git a/test/Dialect/FIRRTL/resolve-paths-errors.mlir b/test/Dialect/FIRRTL/resolve-paths-errors.mlir index 9baee2194bc6..d8bf3c3e3b4e 100644 --- a/test/Dialect/FIRRTL/resolve-paths-errors.mlir +++ b/test/Dialect/FIRRTL/resolve-paths-errors.mlir @@ -74,7 +74,7 @@ firrtl.module @VectorTarget(in %a : !firrtl.vector, 1>) { firrtl.circuit "AmbiguousPath" { firrtl.module @AmbiguousPath() { - // expected-warning @below {{unable to uniquely resolve target due to multiple instantiation}} + // expected-error @below {{unable to uniquely resolve target due to multiple instantiation}} %0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousPath|Child" // expected-note @below {{instance here}} firrtl.instance child0 @Child() diff --git a/test/Dialect/FIRRTL/resolve-paths.mlir b/test/Dialect/FIRRTL/resolve-paths.mlir index 6664d917a649..80d97288f3d4 100644 --- a/test/Dialect/FIRRTL/resolve-paths.mlir +++ b/test/Dialect/FIRRTL/resolve-paths.mlir @@ -225,58 +225,3 @@ firrtl.class @OM() { %0 = firrtl.unresolved_path "OMReferenceTarget:~TargetWire|TargetWire>wire" } } - -// ----- - -// CHECK-LABEL: firrtl.circuit "AmbiguousLocalPath" -firrtl.circuit "AmbiguousLocalPath" { -firrtl.module @AmbiguousLocalPath() { - // CHECK: [[PATH0:%.+]] = firrtl.path reference distinct[[[DISTINCT0:.+]]]<> - %0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousLocalPath|Child" - - // CHECK: firrtl.list.create [[PATH0]] - %1 = firrtl.list.create %0 : !firrtl.list - - // CHECK: firrtl.instance child0 - firrtl.instance child0 @Child() - // CHECK: firrtl.instance child1 - firrtl.instance child1 @Child() -} -// CHECK: firrtl.module @Child -// CHECK-SAME: {class = "circt.tracker", id = distinct[[[DISTINCT0]]]<>} -firrtl.module @Child() {} -} - -// ----- - -// CHECK-LABEL: firrtl.circuit "AmbiguousNonLocalPath" -firrtl.circuit "AmbiguousNonLocalPath" { -// CHECK: hw.hierpath private [[NLA:@.+]] [@Child2::[[SYM0:@.+]], @Child3::[[SYM1:@.+]], @Child4] -firrtl.module @AmbiguousNonLocalPath() { - // CHECK: [[PATH0:%.+]] = firrtl.path reference distinct[[[DISTINCT0:.+]]]<> - %0 = firrtl.unresolved_path "OMReferenceTarget:~AmbiguousNonLocalPath|Child2/child3:Child3/child4:Child4" - - // CHECK: firrtl.list.create [[PATH0]] - %1 = firrtl.list.create %0 : !firrtl.list - - // CHECK: firrtl.instance child1 - firrtl.instance child1 @Child1() -} -firrtl.module @Child1() { - // CHECK: firrtl.instance child2_0 - firrtl.instance child2_0 @Child2() - // CHECK: firrtl.instance child2_1 - firrtl.instance child2_1 @Child2() -} -firrtl.module @Child2() { - // CHECK: firrtl.instance child3 sym [[SYM0]] - firrtl.instance child3 @Child3() -} -firrtl.module @Child3() { - // CHECK: firrtl.instance child4 sym [[SYM1]] - firrtl.instance child4 @Child4() -} -// CHECK: firrtl.module @Child4 -// CHECK-SAME: {circt.nonlocal = [[NLA]], class = "circt.tracker", id = distinct[[[DISTINCT0]]]<>} -firrtl.module @Child4() {} -}