[FIRRTL] Add back unambiguous path requirement. (#7588)

This was made a warning in #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 #7128.

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

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

test/Dialect/FIRRTL/resolve-paths-errors.mlir

@@ -74,7 +74,7 @@ firrtl.module @VectorTarget(in %a : !firrtl.vector<uint<1>, 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()

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<path>
-
- // 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<path>
-
- // 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() {}
-}