diff --git a/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp b/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp index c05a1d9e97..6e7909e9ba 100644 --- a/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp +++ b/lib/Dialect/SV/Transforms/SVExtractTestCode.cpp @@ -18,6 +18,7 @@ #include "circt/Dialect/HW/HWInstanceGraph.h" #include "circt/Dialect/HW/HWOps.h" #include "circt/Dialect/HW/HWSymCache.h" +#include "circt/Dialect/HW/Namespace.h" #include "circt/Dialect/SV/SVPasses.h" #include "circt/Dialect/Seq/SeqDialect.h" #include "circt/Dialect/Seq/SeqOps.h" @@ -332,14 +333,44 @@ static void addExistingBinds(Block *topLevelModule, BindTable &bindTable) { } // Inline any modules that only have inputs for test code. -static void inlineInputOnly(hw::HWModuleOp oldMod, - hw::InstanceGraph &instanceGraph, - BindTable &bindTable, - SmallPtrSetImpl &opsToErase) { +static void +inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceGraph &instanceGraph, + BindTable &bindTable, SmallPtrSetImpl &opsToErase, + llvm::DenseSet &innerRefUsedByNonBindOp) { + // Check if the module only has inputs. if (oldMod.getNumOutputs() != 0) return; + // Check if it's ok to inline. We cannot inline the module if there exists a + // declaration with an inner symbol referred by non-bind ops (e.g. hierpath). + for (auto port : oldMod.getPorts()) { + if (port.sym) { + // Reject if the inner sym is a per-field symbol, or its inner ref is used + // by non-bind op. + if (!port.sym.getSymName() || port.sym.size() != 1 || + innerRefUsedByNonBindOp.count(hw::InnerRefAttr::get( + oldMod.getModuleNameAttr(), port.sym.getSymName()))) { + oldMod.emitWarning() << "module " << oldMod.getModuleName() + << " is an input only module but cannot " + "be inlined because a signal " + << port.name << " is referred by name"; + return; + } + } + } + for (auto &op : *oldMod.getBodyBlock()) { + if (auto innerSym = op.getAttrOfType("inner_sym")) + if (innerRefUsedByNonBindOp.count( + hw::InnerRefAttr::get(oldMod.getModuleNameAttr(), innerSym))) { + op.emitWarning() + << "module " << oldMod.getModuleName() + << " is an input only module but cannot be inlined because signals " + "are referred by name"; + return; + } + } + // Get the instance graph node for the old module. hw::InstanceGraphNode *node = instanceGraph.lookup(oldMod); assert(!node->noUses() && @@ -360,6 +391,11 @@ static void inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceOp inst = cast(instLike.getOperation()); if (inst.getInnerSym().has_value()) { allInlined = false; + auto diag = + oldMod.emitWarning() + << "module " << oldMod.getModuleName() + << " cannot be inlined because there is an instance with a symbol"; + diag.attachNote(inst.getLoc()); continue; } @@ -376,11 +412,24 @@ static void inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceGraphNode *instParentNode = instanceGraph.lookup(instParent); SmallVector lateBoundOps; b.setInsertionPoint(inst); + // Namespace that tracks inner symbols in the parent module. + hw::ModuleNamespace nameSpace(instParent); + // A map from old inner symbols to new ones. + DenseMap symMapping; + for (auto &op : *oldMod.getBodyBlock()) { // If the op was erased by instance extraction, don't copy it over. if (opsToErase.contains(&op)) continue; + // If the op has an inner sym, first create a new inner sym for it. + if (auto innerSym = op.getAttrOfType("inner_sym")) { + auto newName = b.getStringAttr(nameSpace.newName(innerSym.getValue())); + auto result = symMapping.insert({innerSym, newName}); + (void)result; + assert(result.second && "inner symbols must be unique"); + } + // For instances in the bind table, update the bind with the new parent. if (auto innerInst = dyn_cast(op)) { if (auto innerInstSym = innerInst.getInnerSymAttr()) { @@ -388,9 +437,19 @@ static void inlineInputOnly(hw::HWModuleOp oldMod, if (it != bindTable[oldMod.getNameAttr()].end()) { sv::BindOp bind = it->second; auto oldInnerRef = bind.getInstanceAttr(); - auto newInnerRef = hw::InnerRefAttr::get( - instParent.getModuleNameAttr(), oldInnerRef.getName()); - bind.setInstanceAttr(newInnerRef); + auto it = symMapping.find(oldInnerRef.getName()); + assert(it != symMapping.end() && + "inner sym mapping must be already populated"); + auto newName = it->second; + auto newInnerRef = + hw::InnerRefAttr::get(instParent.getModuleNameAttr(), newName); + OpBuilder::InsertionGuard g(b); + // Clone bind operations. + b.setInsertionPoint(bind); + sv::BindOp clonedBind = cast(b.clone(*bind, mapping)); + clonedBind.setInstanceAttr(newInnerRef); + bindTable[instParent.getModuleNameAttr()][newName] = + cast(clonedBind); } } } @@ -410,6 +469,10 @@ static void inlineInputOnly(hw::HWModuleOp oldMod, instanceGraph.lookup(innerInst.getModuleNameAttr().getAttr()); instParentNode->addInstance(innerInst, innerInstModule); } + + // If the op has an inner sym, then attach an updated inner sym. + if (auto innerSym = op.getAttrOfType("inner_sym")) + clonedOp->setAttr("inner_sym", symMapping[innerSym]); } } @@ -424,6 +487,10 @@ static void inlineInputOnly(hw::HWModuleOp oldMod, // If all instances were inlined, remove the module. if (allInlined) { + // Erase old bind statements. + for (auto [_, bind] : bindTable[oldMod.getNameAttr()]) + bind.erase(); + bindTable[oldMod.getNameAttr()].clear(); instanceGraph.erase(node); opsToErase.insert(oldMod); } @@ -610,6 +677,21 @@ void SVExtractTestCodeImplPass::runOnOperation() { this->instanceGraph = &getAnalysis(); auto top = getOperation(); + + // It takes extra effort to inline modules which contains inner symbols + // referred through hierpaths or unknown operations since we have to update + // inner refs users globally. However we do want to inline modules which + // contain bound instances so create a set of inner refs used by non bind op + // in order to allow bind ops. + DenseSet innerRefUsedByNonBindOp; + top.walk([&](Operation *op) { + if (!isa(op)) + for (auto attr : op->getAttrs()) + attr.getValue().walk([&](hw::InnerRefAttr attr) { + innerRefUsedByNonBindOp.insert(attr); + }); + }); + auto *topLevelModule = top.getBody(); auto assertDir = top->getAttrOfType("firrtl.extract.assert"); @@ -724,7 +806,8 @@ void SVExtractTestCodeImplPass::runOnOperation() { // Inline any modules that only have inputs for test code. if (!disableModuleInlining && anyThingExtracted) - inlineInputOnly(rtlmod, *instanceGraph, bindTable, opsToErase); + inlineInputOnly(rtlmod, *instanceGraph, bindTable, opsToErase, + innerRefUsedByNonBindOp); numOpsErased += opsToErase.size(); while (!opsToErase.empty()) { diff --git a/test/Dialect/SV/hw-extract-test-code.mlir b/test/Dialect/SV/hw-extract-test-code.mlir index 224c048fa6..2026b7c66b 100644 --- a/test/Dialect/SV/hw-extract-test-code.mlir +++ b/test/Dialect/SV/hw-extract-test-code.mlir @@ -465,3 +465,54 @@ module { hw.output %true : i1 } } + +// ----- +// Check that input only modules are inlined properly. + +module { + // @ShouldNotBeInlined cannot be inlined because there is a wire with an inner sym + // that is referred by hierpath op. + hw.hierpath private @Foo [@ShouldNotBeInlined::@foo] + hw.module private @ShouldNotBeInlined(%clock: i1, %a: i1) { + %w = sv.wire sym @foo: !hw.inout + sv.always posedge %clock { + sv.if %a { + sv.assert %a, immediate message "foo" + } + } + hw.output + } + hw.module private @Assert(%clock: i1, %a: i1) { + sv.always posedge %clock { + sv.if %a { + sv.assert %a, immediate message "foo" + } + } + hw.output + } + + // CHECK-LABEL: hw.module private @AssertWrapper(%clock: i1, %a: i1) -> (b: i1) { + // CHECK-NEXT: hw.instance "Assert_assert" sym @__ETC_Assert_assert @Assert_assert + // CHECK-SAME: doNotPrint = true + hw.module private @AssertWrapper(%clock: i1, %a: i1) -> (b: i1) { + hw.instance "a3" @Assert(clock: %clock: i1, a: %a: i1) -> () + hw.output %a: i1 + } + + // CHECK-LABEL: hw.module @Top(%clock: i1, %a: i1, %b: i1) { + // CHECK-NEXT: hw.instance "Assert_assert" sym @__ETC_Assert_assert_0 @Assert_assert + // CHECK-SAME: doNotPrint = true + // CHECK-NEXT: hw.instance "Assert_assert" sym @__ETC_Assert_assert @Assert_assert + // CHECK-SAME: doNotPrint = true + // CHECK-NEXT: hw.instance "should_not_be_inlined" @ShouldNotBeInlined + // CHECK-NOT: doNotPrint + hw.module @Top(%clock: i1, %a: i1, %b: i1) { + hw.instance "a1" @Assert(clock: %clock: i1, a: %a: i1) -> () + hw.instance "a2" @Assert(clock: %clock: i1, a: %b: i1) -> () + hw.instance "should_not_be_inlined" @ShouldNotBeInlined (clock: %clock: i1, a: %b: i1) -> () + hw.output + } + // CHECK: sv.bind <@Top::@__ETC_Assert_assert> + // CHECK-NEXT: sv.bind <@Top::@__ETC_Assert_assert_0> + // CHECK-NEXT: sv.bind <@AssertWrapper::@__ETC_Assert_assert> +}