Skip to content

Commit

Permalink
[ModuleInliner] Rework instance rename code, fix issue as w/PR4882. (#…
Browse files Browse the repository at this point in the history 
…4894)

Co-authored-by: Prithayan Barua <[email protected]>
  • Loading branch information
@dtzSiFive @prithayan @uenoku
2 people authored and uenoku committed Apr 14, 2023
1 parent 794b654 commit a587c1a
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 80 deletions.
189 changes: 109 additions & 80 deletions lib/Dialect/FIRRTL/Transforms/ModuleInliner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,13 +507,17 @@ class Inliner {
/// referring to some other path.
bool doesNLAMatchCurrentPath(hw::HierPathOp nla);

/// Rename an operation and unique any symbols it has. If the op is an
/// InstanceOp, then `validHierPaths` is the set of HierPaths that the
/// InstanceOp participates in. `validHierPaths` is required, since the
/// InstanceOp no longer contains the BreadCrumbs which indicated the
/// `HierPathOps` that it participates in.
void rename(StringRef prefix, Operation *op, ModuleNamespace &moduleNamespace,
SmallVector<StringAttr> &validHierPaths);
/// Rename an operation and unique any symbols it has.
/// Returns true iff symbol was changed.
bool rename(StringRef prefix, Operation *op,
ModuleNamespace &moduleNamespace);

/// Rename an InstanceOp and unique any symbols it has.
/// Requires old and new operations to appropriately update the `HierPathOp`'s
/// that it participates in.
bool renameInstance(StringRef prefix, InstanceOp oldInst, InstanceOp newInst,
ModuleNamespace &moduleNamespace,
const DenseMap<Attribute, Attribute> &symbolRenames);

/// Clone and rename an operation.
void cloneAndRename(StringRef prefix, OpBuilder &b, IRMapping &mapper,
Expand Down Expand Up @@ -628,10 +632,8 @@ bool Inliner::doesNLAMatchCurrentPath(hw::HierPathOp nla) {
/// If this operation or any child operation has a name, add the prefix to that
/// operation's name. If the operation has any inner symbols, make sure that
/// these are unique in the namespace.
// NOLINTNEXTLINE(misc-no-recursion)
void Inliner::rename(StringRef prefix, Operation *op,
ModuleNamespace &moduleNamespace,
SmallVector<StringAttr> &validHierPaths) {
bool Inliner::rename(StringRef prefix, Operation *op,
ModuleNamespace &moduleNamespace) {
// Add a prefix to things that has a "name" attribute. We don't prefix
// memories since it will affect the name of the generated module.
// TODO: We should find a way to prefix the instance of a memory module.
Expand All @@ -641,14 +643,6 @@ void Inliner::rename(StringRef prefix, Operation *op,
(prefix + nameAttr.getValue())));
}

// Record the list of HierPathOps that the original op participates in, so
// that they can be moved to the op after renaming.
StringAttr instanceParent, oldInstSym;
if (auto inst = dyn_cast<InstanceOp>(op))
if (auto instSym = getInnerSymName(inst)) {
instanceParent = inst->getParentOfType<FModuleOp>().getNameAttr();
oldInstSym = instSym;
}
// If the operation has an inner symbol, ensure that it is unique. Record
// renames for any NLAs that this participates in if the symbol was renamed.
if (auto sym = getInnerSymName(op)) {
Expand All @@ -670,29 +664,85 @@ void Inliner::rename(StringRef prefix, Operation *op,
continue;
mnla.setInnerSym(moduleNamespace.module.moduleNameAttr(), newSymAttr);
}
if (instanceParent) {
// The InstanceOp is renamed, so move the HierPathOps to the new
// InnerRefAttr.
auto newInnerRef = InnerRefAttr::get(instanceParent, newSymAttr);
instOpHierPaths[newInnerRef] = validHierPaths;
// Update the innerSym for all the affected HierPathOps.
for (auto nla : instOpHierPaths[newInnerRef]) {
if (!nlaMap.count(nla))
continue;
auto &mnla = nlaMap[nla];
mnla.setInnerSym(moduleNamespace.module.moduleNameAttr(), newSymAttr);
}
auto oldInnerRef = InnerRefAttr::get(instanceParent, oldInstSym);
instOpHierPaths.erase(oldInnerRef);
}
// Indicate symbol was changed.
return true;
}
}
// No change to symbol, if present.
return false;
}

bool Inliner::renameInstance(
StringRef prefix, InstanceOp oldInst, InstanceOp newInst,
ModuleNamespace &moduleNamespace,
const DenseMap<Attribute, Attribute> &symbolRenames) {
// List of HierPathOps that are valid based on the InstanceOp being inlined
// and the InstanceOp which is being replaced after inlining. That is the set
// of HierPathOps that is common between these two.
SmallVector<StringAttr> validHierPaths;
auto oldParent = oldInst->getParentOfType<FModuleOp>().getNameAttr();
auto oldInstSym = getInnerSymName(oldInst);

if (oldInstSym) {
// Get the innerRef to the original InstanceOp that is being inlined here.
// For all the HierPathOps that the instance being inlined participates
// in.
auto oldInnerRef = InnerRefAttr::get(oldParent, oldInstSym);
for (auto old : instOpHierPaths[oldInnerRef]) {
// If this HierPathOp is valid at the inlining context, where the
// instance is being inlined at. That is, if it exists in the
// activeHierpaths.
if (activeHierpaths.find(old) != activeHierpaths.end())
validHierPaths.push_back(old);
else
// The HierPathOp could have been renamed, check for the other retoped
// names, if they are active at the inlining context.
for (auto additionalSym : nlaMap[old].getAdditionalSymbols())
if (activeHierpaths.find(additionalSym.getName()) !=
activeHierpaths.end()) {
validHierPaths.push_back(old);
break;
}
}
}

assert(getInnerSymName(newInst) == oldInstSym);

// Do the renaming, creating new symbol as needed.
auto symbolChanged = rename(prefix, newInst, moduleNamespace);

// If the symbol changed, update instOpHierPaths accordingly.
auto newSymAttr = getInnerSymName(newInst);
if (symbolChanged) {
assert(newSymAttr);
// The InstanceOp is renamed, so move the HierPathOps to the new
// InnerRefAttr.
auto newInnerRef = InnerRefAttr::get(
newInst->getParentOfType<FModuleOp>().getNameAttr(), newSymAttr);
instOpHierPaths[newInnerRef] = validHierPaths;
// Update the innerSym for all the affected HierPathOps.
for (auto nla : instOpHierPaths[newInnerRef]) {
if (!nlaMap.count(nla))
continue;
auto &mnla = nlaMap[nla];
assert(newInnerRef.getModule() ==
moduleNamespace.module.moduleNameAttr());
mnla.setInnerSym(moduleNamespace.module.moduleNameAttr(), newSymAttr);
}
}

// Recursively rename any child operations.
for (auto &region : op->getRegions())
for (auto &block : region)
for (auto &op : block)
rename(prefix, &op, moduleNamespace, validHierPaths);
if (newSymAttr) {
auto innerRef = InnerRefAttr::get(
newInst->getParentOfType<FModuleOp>().getNameAttr(), newSymAttr);
SmallVector<StringAttr> &nlaList = instOpHierPaths[innerRef];
// Now rename the Updated HierPathOps that this InstanceOp participates in.
for (const auto &en : llvm::enumerate(nlaList)) {
auto oldNLA = en.value();
if (auto newSym = symbolRenames.lookup(oldNLA))
nlaList[en.index()] = newSym.cast<StringAttr>();
}
}
return symbolChanged;
}

/// This function is used before inlining a module, to handle the conversion
Expand Down Expand Up @@ -802,47 +852,26 @@ void Inliner::cloneAndRename(

// Clone and rename.
auto *newOp = b.clone(op, mapper);
// List of HierPathOps that are valid based on the InstanceOp being inlined
// and the InstanceOp which is being replaced after inlining. That is the set
// of HierPathOps that is common between these two.
SmallVector<StringAttr> validHierPaths;
if (auto instance = dyn_cast<InstanceOp>(&op))
if (auto instSym = getInnerSymName(instance)) {
// Get the innerRef to the original InstanceOp that is being inlined here.
auto oldInnerRef = InnerRefAttr::get(
instance->getParentOfType<FModuleOp>().getNameAttr(), instSym);
// For all the HierPathOps that the instance being inlined participates
// in.
for (auto old : instOpHierPaths[oldInnerRef]) {
// If this HierPathOp is valid at the inlining context, where the
// instance is being inlined at. That is, if it exists in the
// activeHierpaths.
if (activeHierpaths.find(old) != activeHierpaths.end())
validHierPaths.push_back(old);
else
// The HierPathOp could have been renamed, check for the other retoped
// names, if they are active at the inlining context.
for (auto additionalSym : nlaMap[old].getAdditionalSymbols())
if (activeHierpaths.find(additionalSym.getName()) !=
activeHierpaths.end()) {
validHierPaths.push_back(old);
break;
}
}
}
rename(prefix, newOp, moduleNamespace, validHierPaths);
if (isa<InstanceOp>(&op)) {
auto innerRef =
InnerRefAttr::get(newOp->getParentOfType<FModuleOp>().getNameAttr(),
getInnerSymName(newOp));
SmallVector<StringAttr> &nlaList = instOpHierPaths[innerRef];
// Now rename the Updated HierPathOps that this InstanceOp participates in.
for (const auto &en : llvm::enumerate(nlaList)) {
auto oldNLA = en.value();
if (auto newSym = symbolRenames.lookup(oldNLA))
nlaList[en.index()] = newSym.cast<StringAttr>();
}
}

// Rename the new operation and any contained operations.
// (add prefix to it, if named, and unique-ify symbol, updating NLA's).
op.walk<mlir::WalkOrder::PreOrder>([&](Operation *origOp) {
auto *newOpToRename = mapper.lookup(origOp);
assert(newOpToRename);
// TODO: If want to work before ExpandWhen's, more work needed!
// Handle what we can for now.
assert(origOp == &op || !isa<InstanceOp>(origOp) &&
"Cannot handle instances not at top-level");

// Instances require extra handling to update HierPathOp's if their symbols
// change.
if (auto oldInst = dyn_cast<InstanceOp>(origOp))
renameInstance(prefix, oldInst, cast<InstanceOp>(newOpToRename),
moduleNamespace, symbolRenames);
else
rename(prefix, newOpToRename, moduleNamespace);
});

// We want to avoid attaching an empty annotation array on to an op that
// never had an annotation array in the first place.
if (!newAnnotations.empty() || !oldAnnotations.empty())
Expand Down
26 changes: 26 additions & 0 deletions test/Dialect/FIRRTL/inliner.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1113,3 +1113,29 @@ firrtl.circuit "Top" {
firrtl.strictconnect %_a, %1 : !firrtl.ref<uint<1>>
}
}


// -----

// PR #4882 fixes a bug, which was producing invalid NLAs.
// error: 'hw.hierpath' op module: "instNameRename" does not contain any instance with symbol: "w"
// Due to coincidental name collisions, renaming was not updating the actual hierpath.
firrtl.circuit "Bug4882Rename" {
hw.hierpath private @nla_5560 [@Bug4882Rename::@w, @Bar2::@x]
firrtl.module private @Bar2() attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]}{
%x = firrtl.wire sym @x {annotations = [{circt.nonlocal = @nla_5560, class = "test0"}]} : !firrtl.uint<8>
}
firrtl.module private @Bar1() attributes {annotations = [{class = "firrtl.passes.InlineAnnotation"}]}{
firrtl.instance bar3 sym @w @Bar3()
}
firrtl.module private @Bar3() {
%w = firrtl.wire sym @w1 : !firrtl.uint<8>
}
firrtl.module @Bug4882Rename() {
// CHECK-LABEL: firrtl.module @Bug4882Rename() {
firrtl.instance no sym @no @Bar1()
// CHECK-NEXT: firrtl.instance no_bar3 sym @w_0 @Bar3()
firrtl.instance bar2 sym @w @Bar2()
// CHECK-NEXT: %bar2_x = firrtl.wire sym @x {annotations = [{class = "test0"}]}
}
}

0 comments on commit a587c1a

Please sign in to comment.