From c12c68d3a21185132659747c1200958bd5c2329b Mon Sep 17 00:00:00 2001 From: Mike Urbach Date: Mon, 11 Mar 2024 19:25:07 -0600 Subject: [PATCH] [FIRRTL] Don't enforce owning module for local ref in LowerClasses. (#6811) We have an existing check that paths are in the same owning module as the entity they target. This is absolutely required for hierarchical references to compose with basepaths. However, we have legacy flows that extract entities out of their owning module, and we want to be able to target those things. This works around these conflicting requirements by skipping the owning module check for local targets only. This is safe because we still check such local targets are only instantiated once, so there is no ambiguity even though they are not instantiated within the owning module. --- lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp | 11 ++++++++++- test/Dialect/FIRRTL/lower-classes.mlir | 8 ++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp index 98d555859b9f..a857f7d93673 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp @@ -269,8 +269,17 @@ LogicalResult LowerClassesPass::processPaths( if (node->getModule() == owningModule) break; // If there are no more parents, then the path op lives in a different - // hierarchy than the HW object it references, which is an error. + // hierarchy than the HW object it references, which is an error if the + // reference is hierarchical. We allow local targets in this case + // because we intentionally extract some things out of the owning module + // for legacy reasons, and expect to target them with local targets. + // Once that is fixed, we can restore the invariant that the path op + // lives in the same hierarchy as the HW object it references. if (node->noUses()) { + // Local target temporary special case. + if (!hierPathOp) + break; + op->emitError() << "unable to resolve path relative to owning module " << owningModule.getModuleNameAttr(); error = true; diff --git a/test/Dialect/FIRRTL/lower-classes.mlir b/test/Dialect/FIRRTL/lower-classes.mlir index 2b270121b354..fcfab88c70cc 100644 --- a/test/Dialect/FIRRTL/lower-classes.mlir +++ b/test/Dialect/FIRRTL/lower-classes.mlir @@ -154,6 +154,7 @@ firrtl.circuit "Component" { // CHECK-LABEL: firrtl.circuit "PathModule" firrtl.circuit "PathModule" { + // CHECK: hw.hierpath private [[LOCAL_PATH_NOT_OWNED:@.+]] [@NotOwned::@wire] // CHECK: hw.hierpath private [[PORT_PATH:@.+]] [@PathModule::[[PORT_SYM:@.+]]] // CHECK: hw.hierpath private [[WIRE_PATH:@.+]] [@PathModule::[[WIRE_SYM:@.+]]] // CHECK: hw.hierpath private [[VECTOR_PATH:@.+]] [@PathModule::[[VECTOR_SYM:@.+]]] @@ -161,6 +162,10 @@ firrtl.circuit "PathModule" { // CHECK: hw.hierpath private [[MODULE_PATH:@.+]] [@Child] // CHECK: hw.hierpath private [[NONLOCAL_PATH:@.+]] [@PathModule::@child, @Child::[[NONLOCAL_SYM:@.+]]] + firrtl.module @NotOwned() { + %wire = firrtl.wire sym @wire {annotations = [{class = "circt.tracker", id = distinct[6]<>}]} : !firrtl.uint<1> + } + // CHECK: firrtl.module @PathModule(in %in: !firrtl.uint<1> sym [[PORT_SYM]]) { firrtl.module @PathModule(in %in : !firrtl.uint<1> [{class = "circt.tracker", id = distinct[0]<>}]) { // CHECK: %wire = firrtl.wire sym [[WIRE_SYM]] : !firrtl.uint<8> @@ -211,6 +216,9 @@ firrtl.circuit "PathModule" { // CHECK: om.path_create reference %basepath [[MODULE_PATH]] %module_path = firrtl.path reference distinct[5]<> + + // CHECK: om.path_create reference %basepath [[LOCAL_PATH_NOT_OWNED]] + %local_path_not_owned = firrtl.path reference distinct[6]<> } firrtl.module @ListCreate(in %propIn: !firrtl.integer, out %propOut: !firrtl.list) attributes {convention = #firrtl} { %0 = firrtl.integer 123