Skip to content

Commit

Permalink
[FIRRTL] Only add Seq Mem Ports in the design
Browse files Browse the repository at this point in the history
Tighten the restrictions on when ports are added to memories.  Change this
to only occur for memories that are in the design.  Previously, this was
restricted to memories which were not under layers.  This makes the
behavior of this pass consistent across different non-design features and
this behavior can be easily extended simply by changing what the
InstanceInfo analysis deems is non-design.

As part of this, the entire logic for checking if a memory is legal to add
ports to can be collapsed to checking if it is in the effective design.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
  • Loading branch information
seldridge committed Nov 19, 2024
1 parent b551ee8 commit da2c0f5
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 38 deletions.
43 changes: 8 additions & 35 deletions lib/Dialect/FIRRTL/Transforms/AddSeqMemPorts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,36 +176,10 @@ LogicalResult AddSeqMemPortsPass::processAnnos(CircuitOp circuit) {

LogicalResult AddSeqMemPortsPass::processMemModule(FMemModuleOp mem) {
// Error if all instances are not under the effective DUT.
if (!instanceInfo->allInstancesUnderEffectiveDut(mem)) {
if (!instanceInfo->allInstancesInEffectiveDesign(mem)) {
auto diag = mem->emitOpError()
<< "cannot have ports added to it because it is instantiated "
"both under and not under the design-under-test (DUT)";
for (auto *instNode : instanceGraph->lookup(mem)->uses()) {
auto instanceOp = instNode->getInstance();
if (instanceInfo->anyInstanceUnderDut(
instNode->getParent()->getModule())) {
diag.attachNote(instanceOp.getLoc())
<< "this instance is under the DUT";
continue;
}
diag.attachNote(instanceOp.getLoc())
<< "this instance is not under the DUT";
}
return failure();
}

// Error if the memory is partially under a layer.
//
// TODO: There are several ways to handle a memory being under a layer:
// duplication or tie-off. However, it is unclear if either if these is
// intended or correct. Additionally, this can be handled with pass order by
// preventing deduplication of memories that have this property in
// `LowerMemories`.
if (instanceInfo->anyInstanceUnderLayer(mem)) {
auto diag = mem->emitOpError()
<< "cannot have ports added to it because it is "
"instantiated under "
"both the design-under-test and layer blocks";
for (auto *instNode : instanceGraph->lookup(mem)->uses()) {
auto instanceOp = instNode->getInstance();
if (auto layerBlockOp = instanceOp->getParentOfType<LayerBlockOp>()) {
Expand All @@ -215,11 +189,11 @@ LogicalResult AddSeqMemPortsPass::processMemModule(FMemModuleOp mem) {
<< "the innermost layer block is here";
continue;
}
if (instanceInfo->anyInstanceUnderLayer(
if (!instanceInfo->allInstancesInEffectiveDesign(
instNode->getParent()->getModule())) {
diag.attachNote(instanceOp.getLoc())
<< "this instance is inside a module that is instantiated "
"under a layer block";
<< "this instance is inside a module that is instantiated outside "
"the design";
}
}
return failure();
Expand Down Expand Up @@ -485,15 +459,14 @@ void AddSeqMemPortsPass::runOnOperation() {
numAddedPorts += userPorts.size();

// Visit the modules in post-order starting from the effective
// design-under-test. Skip any modules that are wholly instantiated under
// layers. If any memories are partially instantiated under a layer then
// error.
// design-under-test. Skip any modules that are wholly outside the design.
// If any memories are partially inside and outside the design then error.
for (auto *node : llvm::post_order(
instanceGraph->lookup(instanceInfo->getEffectiveDut()))) {
auto op = node->getModule();

// Skip anything wholly under a layer.
if (instanceInfo->allInstancesUnderLayer(op))
// Skip anything wholly _not_ in the design.
if (!instanceInfo->anyInstanceInEffectiveDesign(op))
continue;

// Process the module or memory.
Expand Down
5 changes: 2 additions & 3 deletions test/Dialect/FIRRTL/add-seqmem-ports-errors.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ firrtl.circuit "LayerBlock" attributes {
}

firrtl.module @Bar() {
// expected-note @below {{this instance is inside a module that is instantiated under a layer block}}
// expected-note @below {{this instance is inside a module that is instantiated outside the design}}
%0:4 = firrtl.instance mem @mem(
in W0_addr: !firrtl.uint<1>,
in W0_en: !firrtl.uint<1>,
Expand Down Expand Up @@ -226,7 +226,6 @@ firrtl.circuit "Foo" attributes {
{class = "sifive.enterprise.firrtl.MarkDUTAnnotation"}
]
} {
// expected-note @below {{this instance is under the DUT}}
%0:4 = firrtl.instance mem @mem(
in waddr: !firrtl.uint<1>,
in wen: !firrtl.uint<1>,
Expand All @@ -237,7 +236,7 @@ firrtl.circuit "Foo" attributes {

firrtl.module @Foo() {
firrtl.instance bar @Bar()
// expected-note @below {{this instance is not under the DUT}}
// expected-note @below {{this instance is inside a module that is instantiated outside the design}}
%0:4 = firrtl.instance mem @mem(
in waddr: !firrtl.uint<1>,
in wen: !firrtl.uint<1>,
Expand Down

0 comments on commit da2c0f5

Please sign in to comment.