Skip to content

Commit

Permalink
[FIRRTL] Update HierPathOps in LowerLayers
Browse files Browse the repository at this point in the history 
Fix a bug in the LowerLayers pass where operations with inner symbols
inside layerblocks would not have their HierPathOp users updated when
these operations were moved into new modules.  Fix this by recording a
mapping of modifications to InnerRefAttrs and then applying these to
HierPathOps after all modules are processed.

Fixes #6717.

Signed-off-by: Schuyler Eldridge <[email protected]>
  • Loading branch information
@seldridge
seldridge committed Feb 19, 2024
1 parent 79c90ac commit 35e3ea1
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 10 deletions.
96 changes: 86 additions & 10 deletions lib/Dialect/FIRRTL/Transforms/LowerLayers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "circt/Dialect/FIRRTL/FIRRTLUtils.h"
#include "circt/Dialect/FIRRTL/Namespace.h"
#include "circt/Dialect/FIRRTL/Passes.h"
#include "circt/Dialect/HW/InnerSymbolNamespace.h"
#include "circt/Dialect/SV/SVOps.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/Mutex.h"
Expand Down Expand Up @@ -56,17 +57,23 @@ struct ConnectInfo {

} // namespace

// A mapping of an old InnerRefAttr to the new inner symbol and module name that
// need to be spliced into the old InnerRefAttr. This is used to fix
// hierarchical path operations after layers are converted to modules.
using InnerRefMap =
DenseMap<hw::InnerRefAttr, std::pair<hw::InnerSymAttr, StringAttr>>;

class LowerLayersPass : public LowerLayersBase<LowerLayersPass> {
/// Safely build a new module with a given namehint. This handles geting a
/// lock to modify the top-level circuit.
FModuleOp buildNewModule(OpBuilder &builder, Location location,
Twine namehint, SmallVectorImpl<PortInfo> &ports);

/// Strip layer colors from the module's interface.
void runOnModuleLike(FModuleLike moduleLike);
InnerRefMap runOnModuleLike(FModuleLike moduleLike);

/// Extract layerblocks and strip probe colors from all ops under the module.
void runOnModuleBody(FModuleOp moduleOp);
void runOnModuleBody(FModuleOp moduleOp, InnerRefMap &innerRefMap);

/// Update the module's port types to remove any explicit layer requirements
/// from any probe types.
Expand Down Expand Up @@ -156,30 +163,35 @@ void LowerLayersPass::removeLayersFromPorts(FModuleLike moduleLike) {
}
}

void LowerLayersPass::runOnModuleLike(FModuleLike moduleLike) {
InnerRefMap LowerLayersPass::runOnModuleLike(FModuleLike moduleLike) {
LLVM_DEBUG({
llvm::dbgs() << "Module: " << moduleLike.getModuleName() << "\n";
llvm::dbgs() << " Examining Layer Blocks:\n";
});

// Strip away layers from the interface of the module-like op.
InnerRefMap innerRefMap;
TypeSwitch<Operation *, void>(moduleLike.getOperation())
.Case<FModuleOp>([&](auto op) {
op.setLayers({});
removeLayersFromPorts(op);
runOnModuleBody(op);
runOnModuleBody(op, innerRefMap);
})
.Case<FExtModuleOp, FIntModuleOp, FMemModuleOp>([&](auto op) {
op.setLayers({});
removeLayersFromPorts(op);
})
.Case<ClassOp, ExtClassOp>([](auto) {})
.Default([](auto) { assert(0 && "unknown module-like op"); });

return innerRefMap;
}

void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp) {
void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp,
InnerRefMap &innerRefMap) {
CircuitOp circuitOp = moduleOp->getParentOfType<CircuitOp>();
StringRef circuitName = circuitOp.getName();
hw::InnerSymbolNamespace ns(moduleOp);

// A map of instance ops to modules that this pass creates. This is used to
// check if this was an instance that we created and to do fast module
Expand Down Expand Up @@ -333,7 +345,14 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp) {
builder.create<RefSendOp>(loc, src)->getResult(0));
};

SmallVector<hw::InnerSymAttr> innerSyms;
for (auto &op : llvm::make_early_inc_range(*body)) {
// Record any operations inside the layer block which have inner symbols.
// Theses may have symbol users which need to be updated.
if (auto symOp = dyn_cast<hw::InnerSymbolOpInterface>(op))
if (auto innerSym = symOp.getInnerSymAttr())
innerSyms.push_back(innerSym);

// Handle instance ops that were created from nested layer blocks. These
// ops need to be moved outside the layer block to avoid nested binds.
// Nested binds are illegal in the SystemVerilog specification (and
Expand Down Expand Up @@ -458,13 +477,18 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp) {
// instance everytime it is moved into a parent layer.
builder.setInsertionPointAfter(layerBlock);
auto moduleName = newModule.getModuleName();
auto instanceName =
(Twine((char)tolower(moduleName[0])) + moduleName.drop_front()).str();
auto instanceOp = builder.create<InstanceOp>(
layerBlock.getLoc(), /*moduleName=*/newModule,
/*name=*/
(Twine((char)tolower(moduleName[0])) + moduleName.drop_front()).str(),
NameKindEnum::DroppableName,
instanceName, NameKindEnum::DroppableName,
/*annotations=*/ArrayRef<Attribute>{},
/*portAnnotations=*/ArrayRef<Attribute>{}, /*lowerToBind=*/true);
/*portAnnotations=*/ArrayRef<Attribute>{}, /*lowerToBind=*/true,
/*innerSym=*/
(innerSyms.empty() ? hw::InnerSymAttr{}
: hw::InnerSymAttr::get(builder.getStringAttr(
ns.newName(instanceName)))));
instanceOp->setAttr("output_file",
hw::OutputFileAttr::getFromFilename(
builder.getContext(),
Expand All @@ -473,6 +497,18 @@ void LowerLayersPass::runOnModuleBody(FModuleOp moduleOp) {
/*excludeFromFileList=*/true));
createdInstances.try_emplace(instanceOp, newModule);

LLVM_DEBUG(llvm::dbgs() << " moved inner refs:\n");
for (hw::InnerSymAttr innerSym : innerSyms) {
auto oldInnerRef = hw::InnerRefAttr::get(moduleOp.getModuleNameAttr(),
innerSym.getSymName());
auto splice = std::make_pair(instanceOp.getInnerSymAttr(),
newModule.getModuleNameAttr());
innerRefMap.insert({oldInnerRef, splice});
LLVM_DEBUG(llvm::dbgs() << " - ref: " << oldInnerRef << "\n"
<< " splice: " << splice.first << ", "
<< splice.second << "\n";);
}

// Connect instance ports to values.
assert(ports.size() == connectValues.size() &&
"the number of instance ports and values to connect to them must be "
Expand Down Expand Up @@ -533,11 +569,51 @@ void LowerLayersPass::runOnOperation() {
});
});

auto mergeMaps = [](auto &&a, auto &&b) {
for (auto bb : b)
a.insert(bb);
return std::forward<decltype(a)>(a);
};

// Lower the layer blocks of each module.
SmallVector<FModuleLike> modules(
circuitOp.getBodyBlock()->getOps<FModuleLike>());
parallelForEach(modules,
[this](FModuleLike module) { runOnModuleLike(module); });
auto innerRefMap =
transformReduce(circuitOp.getContext(), modules, InnerRefMap{}, mergeMaps,
[this](FModuleLike mod) { return runOnModuleLike(mod); });

// Rewrite any hw::HierPathOps which have namepaths that contain rewritting
// inner refs.
//
// TODO: This unnecessarily computes a new namepath for every hw::HierPathOp
// even if that namepath is not used. It would be better to only build the
// new namepath when a change is needed, e.g., by recording updates to the
// namepath.
for (hw::HierPathOp hierPathOp : circuitOp.getOps<hw::HierPathOp>()) {
SmallVector<Attribute> newNamepath;
bool modified = false;
for (auto attr : hierPathOp.getNamepath()) {
hw::InnerRefAttr innerRef = dyn_cast<hw::InnerRefAttr>(attr);
if (!innerRef) {
newNamepath.push_back(attr);
continue;
}
auto it = innerRefMap.find(innerRef);
if (it == innerRefMap.end()) {
newNamepath.push_back(attr);
continue;
}

auto &[inst, mod] = it->getSecond();
newNamepath.push_back(
hw::InnerRefAttr::get(innerRef.getModule(), inst.getSymName()));
newNamepath.push_back(hw::InnerRefAttr::get(mod, innerRef.getName()));
modified = true;
}
if (modified)
hierPathOp.setNamepathAttr(
ArrayAttr::get(circuitOp.getContext(), newNamepath));
}

// Generate the header and footer of each bindings file. The body will be
// populated later when binds are exported to Verilog. This produces text
Expand Down
34 changes: 34 additions & 0 deletions test/Dialect/FIRRTL/lower-layers.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -544,3 +544,37 @@ firrtl.circuit "CaptureHardwareMultipleTimes" {
}
}
}

// -----
// HierPathOps are rewritten when operations with inner symbols inside
// layerblocks are moved into new modules.
// https://github.com/llvm/circt/issues/6717

firrtl.circuit "HierPathOps" {
hw.hierpath @nla1 [@Foo::@foo_A]
hw.hierpath @nla2 [@Foo::@bar, @Bar]
hw.hierpath private @nla3 [@Foo::@baz]
firrtl.layer @A bind {}
firrtl.module @Bar() {}
firrtl.module @Foo() {
%0 = firrtl.wire sym @foo_A : !firrtl.uint<1>
firrtl.layerblock @A {
firrtl.instance bar sym @bar @Bar()
%1 = firrtl.wire sym @baz : !firrtl.uint<1>
}
}
}

// CHECK-LABEL: firrtl.circuit "HierPathOps"
//
// CHECK: hw.hierpath @nla1 [@Foo::@foo_A]
// CHECK-NEXT: hw.hierpath @nla2 [@Foo::@[[inst_sym:[_A-Za-z0-9]+]], @[[mod_sym:[_A-Za-z0-9]+]]::@bar, @Bar]
// CHECK-NEXT: hw.hierpath private @nla3 [@Foo::@[[inst_sym]], @[[mod_sym]]::@baz]
//
// CHECK: firrtl.module {{.*}} @[[mod_sym]]()
// CHECK-NEXT: firrtl.instance bar sym @bar
// CHECK-NEXT: firrtl.wire sym @baz
//
// CHECK: firrtl.module @Foo()
// CHECK-NEXT: firrtl.wire sym @foo_A :
// CHECK-NEXT: firrtl.instance {{.*}} sym @[[inst_sym]]

0 comments on commit 35e3ea1

Please sign in to comment.