Skip to content

Commit

Permalink
[FIRRTL] Don't enforce owning module for local ref in LowerClasses. (l…
Browse files Browse the repository at this point in the history
…lvm#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.
  • Loading branch information
mikeurbach authored Mar 12, 2024
1 parent 4d9033e commit c12c68d
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
11 changes: 10 additions & 1 deletion lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions test/Dialect/FIRRTL/lower-classes.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,18 @@ 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:@.+]]]
// CHECK: hw.hierpath private [[INST_PATH:@.+]] [@PathModule::@child]
// 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>
Expand Down Expand Up @@ -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<integer>) attributes {convention = #firrtl<convention scalarized>} {
%0 = firrtl.integer 123
Expand Down

0 comments on commit c12c68d

Please sign in to comment.