terapines
/
circt
mirror of https://github.com/llvm/circt.git
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[SVExtractTestCode] Fix a bug that instance inlining doesn't update inner symbols and clone sv.bind (#5679) 

Close https://github.com/llvm/circt/issues/5665. This commit fixes a bug that
bound instances are accidentally erased. When inlining input only modules it's necessary
to clone bind statements for bound instances in the module. Previously single bind op is shared by multiple instances, as a result ExportVerilog only emits a bind statement for one of them.
This commit is contained in:
Hideto Ueno 2023-07-26 16:18:17 +09:00 committed by GitHub
parent bf7d93f28e
commit feebfdce84
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 142 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
lib/Dialect/SV/Transforms/SVExtractTestCode.cpp
View File

@ -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<Operation *> &opsToErase) {
static void
inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceGraph &instanceGraph,
BindTable &bindTable, SmallPtrSetImpl<Operation *> &opsToErase,
llvm::DenseSet<hw::InnerRefAttr> &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<StringAttr>("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<hw::InstanceOp>(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<Operation *, 16> 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<mlir::StringAttr, mlir::StringAttr> 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<StringAttr>("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<hw::InstanceOp>(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<sv::BindOp>(b.clone(*bind, mapping));
clonedBind.setInstanceAttr(newInnerRef);
bindTable[instParent.getModuleNameAttr()][newName] =
cast<sv::BindOp>(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<StringAttr>("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<circt::hw::InstanceGraph>();
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<hw::InnerRefAttr> innerRefUsedByNonBindOp;
top.walk([&](Operation *op) {
if (!isa<sv::BindOp>(op))
for (auto attr : op->getAttrs())
attr.getValue().walk([&](hw::InnerRefAttr attr) {
innerRefUsedByNonBindOp.insert(attr);
});
});
auto *topLevelModule = top.getBody();
auto assertDir =
top->getAttrOfType<hw::OutputFileAttr>("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()) {

51
test/Dialect/SV/hw-extract-test-code.mlir
View File

@ -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<i1>
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>
}