From 804cdbe7a31a747f57183d876d9a7e8197fa4e2e Mon Sep 17 00:00:00 2001 From: Mike Urbach Date: Thu, 5 Sep 2024 13:25:19 -0600 Subject: [PATCH] [FIRRTL] Add back unambiguous path requirement. (#7588) This was made a warning in https://github.com/llvm/circt/pull/7129. In reality, the flows can fail during LowerClasses if we do allow this through, so this adds back the previous restrictions so we ensure from the start that we have unambiguous paths as originally intended. Closes https://github.com/llvm/circt/issues/7128. --- .../FIRRTL/Transforms/LowerClasses.cpp | 14 ++--- .../FIRRTL/Transforms/ResolvePaths.cpp | 10 ++-- test/Dialect/FIRRTL/resolve-paths-errors.mlir | 2 +- test/Dialect/FIRRTL/resolve-paths.mlir | 55 ------------------- 4 files changed, 10 insertions(+), 71 deletions(-) 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() {} -}