Skip to content

Commit

Permalink
[FIRRTL] Initial support for classes and objects in Dedup. (#6582)
Browse files Browse the repository at this point in the history 
This adds initial support for Dedup to handle deduping classes and
objects. For the most part, this amounts to ensuring core
functionality is implemented in terms of the FModuleLike and
FInstanceLike interfaces, which classes and objects implement,
respectively.

There are a couple places where some specific logic is added for
objects, similar to the specific logic that was already there for
instance ops.

Finally, a small change related to paths is needed in
LowerClasses. There is some logic there that confirms a hierarchical
path's root module is contained within the path's owning module. In
the case of deduping hierarchical paths, an extra layer of hierarchy
is added to hierpath ops, which breaks this logic. A new condition is
added, and if the owning module is anywhere in the hierarchical path,
it is used as the root module.

An end-to-end firtool test is added to demonstrate that classes and
objects dedup, and the final paths are valid in both the local and
hierarchical cases.

This initial support is capable of generating valid paths, but does
not actually confirm that paths which dedup point to entities that
dedup. Comments have been left, and a ticket opened to address this:
#6583.

This initial support does not handle references to objects in class
ports, in the case the objects' classes are deduped. A ticket has also
been opened to address this:
#6603
  • Loading branch information
@mikeurbach
mikeurbach authored Jan 24, 2024
1 parent cb19b54 commit 19d522f
Show file tree
Hide file tree
Showing 3 changed files with 194 additions and 28 deletions.
69 changes: 47 additions & 22 deletions lib/Dialect/FIRRTL/Transforms/Dedup.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ struct StructuralHasherSharedConstants {
moduleNameAttr = StringAttr::get(context, "moduleName");
innerSymAttr = StringAttr::get(context, "inner_sym");
portSymsAttr = StringAttr::get(context, "portSyms");
portNamesAttr = StringAttr::get(context, "portNames");
nonessentialAttributes.insert(StringAttr::get(context, "annotations"));
nonessentialAttributes.insert(StringAttr::get(context, "name"));
nonessentialAttributes.insert(StringAttr::get(context, "portAnnotations"));
Expand All @@ -104,6 +105,9 @@ struct StructuralHasherSharedConstants {
// This is a cached "portSyms" string attr.
StringAttr portSymsAttr;

// This is a cached "portNames" string attr.
StringAttr portNamesAttr;

// This is a set of every attribute we should ignore.
DenseSet<Attribute> nonessentialAttributes;
};
Expand Down Expand Up @@ -159,6 +163,15 @@ struct StructuralHasher {

void update(OpResult result) {
record(result.getAsOpaquePointer());

// Like instance ops, don't use object ops' result types since they might be
// replaced by dedup. Record the class names and lazily combine their hashes
// using the same mechanism as instances and modules.
if (auto objectOp = dyn_cast<ObjectOp>(result.getOwner())) {
referredModuleNames.push_back(objectOp.getType().getNameAttr().getAttr());
return;
}

update(result.getType());
}

Expand Down Expand Up @@ -201,8 +214,11 @@ struct StructuralHasher {
for (auto namedAttr : dict) {
auto name = namedAttr.getName();
auto value = namedAttr.getValue();
// Skip names and annotations.
if (constants.nonessentialAttributes.contains(name))
// Skip names and annotations, except in certain cases.
// Names of ports are load bearing for classes, so we do hash those.
bool isClassPortNames =
isa<ClassLike>(op) && name == constants.portNamesAttr;
if (constants.nonessentialAttributes.contains(name) && !isClassPortNames)
continue;

// Hash the port types.
Expand Down Expand Up @@ -247,6 +263,11 @@ struct StructuralHasher {
// Hash the interned pointer.
update(name.getAsOpaquePointer());

// TODO: properly handle DistinctAttr, including its use in paths.
// See https://github.com/llvm/circt/issues/6583.
if (isa<DistinctAttr>(value))
continue;

// If this is an symbol reference, we need to perform name erasure.
if (auto innerRef = dyn_cast<hw::InnerRefAttr>(value))
update(innerRef);
Expand Down Expand Up @@ -588,6 +609,9 @@ struct Equivalence {
if (failed(check(diag, a, cast<IntegerAttr>(aAttr), b,
cast<IntegerAttr>(bAttr))))
return failure();
} else if (isa<DistinctAttr>(aAttr) && isa<DistinctAttr>(bAttr)) {
// TODO: properly handle DistinctAttr, including its use in paths.
// See https://github.com/llvm/circt/issues/6583
} else if (aAttr != bAttr) {
diag.attachNote(a->getLoc())
<< "first operation has attribute '" << attrName.getValue()
Expand Down Expand Up @@ -617,21 +641,22 @@ struct Equivalence {
}

// NOLINTNEXTLINE(misc-no-recursion)
LogicalResult check(InFlightDiagnostic &diag, InstanceOp a, InstanceOp b) {
auto aName = a.getModuleNameAttr().getAttr();
auto bName = b.getModuleNameAttr().getAttr();
LogicalResult check(InFlightDiagnostic &diag, FInstanceLike a,
FInstanceLike b) {
auto aName = a.getReferencedModuleNameAttr();
auto bName = b.getReferencedModuleNameAttr();
if (aName == bName)
return success();

// If the modules instantiate are different we will want to know why the
// sub module did not dedupliate. This code recursively checks the child
// module.
auto aModule = a.getReferencedModule(instanceGraph);
auto bModule = b.getReferencedModule(instanceGraph);
auto aModule = instanceGraph.lookup(aName)->getModule();
auto bModule = instanceGraph.lookup(bName)->getModule();
// Create a new error for the submodule.
diag.attachNote(std::nullopt)
<< "in instance " << a.getNameAttr() << " of " << aName
<< ", and instance " << b.getNameAttr() << " of " << bName;
<< "in instance " << a.getInstanceNameAttr() << " of " << aName
<< ", and instance " << b.getInstanceNameAttr() << " of " << bName;
check(diag, aModule, bModule);
return failure();
}
Expand All @@ -648,8 +673,8 @@ struct Equivalence {

// If its an instance operaiton, perform some checking and possibly
// recurse.
if (auto aInst = dyn_cast<InstanceOp>(a)) {
auto bInst = cast<InstanceOp>(b);
if (auto aInst = dyn_cast<FInstanceLike>(a)) {
auto bInst = cast<FInstanceLike>(b);
if (failed(check(diag, aInst, bInst)))
return failure();
}
Expand Down Expand Up @@ -940,9 +965,15 @@ struct Deduper {
auto *toNode = instanceGraph[toModule];
auto toModuleRef = FlatSymbolRefAttr::get(toModule.getModuleNameAttr());
for (auto *oldInstRec : llvm::make_early_inc_range(fromNode->uses())) {
auto inst = ::cast<InstanceOp>(*oldInstRec->getInstance());
inst.setModuleNameAttr(toModuleRef);
inst.setPortNamesAttr(toModule.getPortNamesAttr());
auto inst = oldInstRec->getInstance();
if (auto instOp = dyn_cast<InstanceOp>(*inst)) {
instOp.setModuleNameAttr(toModuleRef);
instOp.setPortNamesAttr(toModule.getPortNamesAttr());
} else if (auto objectOp = dyn_cast<ObjectOp>(*inst)) {
auto classLike = cast<ClassLike>(*toNode->getModule());
ClassType classType = detail::getInstanceTypeForClassLike(classLike);
objectOp.getResult().setType(classType);
}
oldInstRec->getParent()->addInstance(inst, toNode);
oldInstRec->erase();
}
Expand Down Expand Up @@ -1523,14 +1554,8 @@ class DedupPass : public DedupBase<DedupPass> {
// If module has symbol (name) that must be preserved even if unused,
// skip it. All symbol uses must be supported, which is not true if
// non-private.
if (!module.isPrivate() || !module.canDiscardOnUseEmpty()) {
return success();
}

// Explicitly skip class-like modules. This is presently unreachable
// due to above and current implementation but check anyway as dedup
// code does not handle these or object operations.
if (isa<ClassLike>(*module)) {
if (!module.isPrivate() ||
(!module.canDiscardOnUseEmpty() && !isa<ClassLike>(*module))) {
return success();
}

Expand Down
37 changes: 31 additions & 6 deletions lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,19 +204,16 @@ LogicalResult LowerClassesPass::processPaths(

auto moduleName = target.getModule().getModuleNameAttr();

// Copy the middle part from the annotation's NLA.
// Verify a nonlocal annotation refers to a HierPathOp.
hw::HierPathOp hierPathOp;
if (auto hierName = anno.getMember<FlatSymbolRefAttr>("circt.nonlocal")) {
auto hierPathOp =
hierPathOp =
dyn_cast<hw::HierPathOp>(symbolTable.lookup(hierName.getAttr()));
if (!hierPathOp) {
op->emitError("annotation does not point at a HierPathOp");
error = true;
return false;
}
// Copy the old path, dropping the module name.
auto oldPath = hierPathOp.getNamepath().getValue();
llvm::append_range(path, llvm::reverse(oldPath.drop_back()));
moduleName = cast<hw::InnerRefAttr>(oldPath.front()).getModule();
}

auto [it, inserted] = pathInfoTable.try_emplace(id);
Expand All @@ -236,6 +233,34 @@ LogicalResult LowerClassesPass::processPaths(
if (!owningModule)
return true;

// Copy the middle part from the annotation's NLA.
if (hierPathOp) {
// Copy the old path, dropping the module name.
auto oldPath = hierPathOp.getNamepath().getValue();
llvm::append_range(path, llvm::reverse(oldPath.drop_back()));

// Set the moduleName based on the hierarchical path. If the
// owningModule is in the hierarichal path, set the moduleName to the
// owning module. Otherwise use the top of the hierarchical path.
bool pathContainsOwningModule =
llvm::any_of(oldPath, [&](auto pathFragment) {
return llvm::TypeSwitch<Attribute, bool>(pathFragment)
.Case([&](hw::InnerRefAttr innerRef) {
return innerRef.getModule() ==
owningModule.getModuleNameAttr();
})
.Case([&](FlatSymbolRefAttr symRef) {
return symRef.getAttr() == owningModule.getModuleNameAttr();
})
.Default([](auto attr) { return false; });
});
if (pathContainsOwningModule) {
moduleName = owningModule.getModuleNameAttr();
} else {
moduleName = cast<hw::InnerRefAttr>(oldPath.front()).getModule();
}
}

// Copy the leading part of the hierarchical path from the owning module
// to the start of the annotation's NLA.
auto *node = instanceGraph.lookup(moduleName);
Expand Down
116 changes: 116 additions & 0 deletions test/firtool/classes-dedupe.fir
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
; RUN: firtool %s -ir-verilog | FileCheck %s

FIRRTL version 3.3.0

circuit Test : %[[
{
"class": "firrtl.transforms.MustDeduplicateAnnotation",
"modules": ["~Test|CPU_1", "~Test|CPU_2"]
}
]]
; CHECK: hw.hierpath private [[NLA1:@.+]] [@Test::@sym, @CPU_1::[[SYM1:@.+]]]
; CHECK: hw.hierpath private [[NLA2:@.+]] [@Test::@sym, @CPU_1::[[SYM2:@.+]], @Fetch_1::[[SYM3:@.+]]]
module Test :
input in : UInt<1>
output out_1 : UInt<1>
output out_2 : UInt<1>
output om_out_1 : AnyRef
output om_out_2 : AnyRef
inst cpu_1 of CPU_1
inst cpu_2 of CPU_2
connect cpu_1.in, in
connect cpu_2.in, in
connect out_1, cpu_1.out
connect out_2, cpu_2.out
propassign om_out_1, cpu_1.om_out
propassign om_out_2, cpu_2.om_out

; CHECK-LABEL: hw.module private @CPU_1
; CHECK-SAME: out out : i1 {hw.exportPort = #hw<innerSym[[SYM1]]>}
module CPU_1 :
input in : UInt<1>
output out : UInt<1>
output om_out : AnyRef

object om of OM_1
propassign om_out, om

; CHECK: hw.instance "fetch_1" sym [[SYM2]]
inst fetch_1 of Fetch_1
inst fetch_2 of Fetch_1
connect fetch_1.in, in
connect fetch_2.in, in
connect out, fetch_1.out

; CHECK-NOT: CPU_2
module CPU_2 :
input in : UInt<1>
output out : UInt<1>
output om_out : AnyRef

object om of OM_2
propassign om_out, om

inst fetch_1 of Fetch_2
inst fetch_2 of Fetch_2
connect fetch_1.in, in
connect fetch_2.in, in
connect out, fetch_1.out

module Fetch_1 :
input in : UInt<1>
output out : UInt<1>
; CHECK: %foo = sv.wire sym [[SYM3]]
wire foo : UInt<1>
connect foo, in
connect out, foo

; CHECK-NOT: Fetch_2
module Fetch_2 :
input in : UInt<1>
output out : UInt<1>
wire foo : UInt<1>
connect foo, in
connect out, foo

class Foo_1 :
output out_foo : Integer
propassign out_foo, Integer(1)

class Foo_2 :
output out_bar : Integer
propassign out_bar, Integer(1)

; CHECK-LABEL: om.class @OM_1(%basepath: !om.basepath)
class OM_1 :
output out_1 : Path
output out_2 : Path
output out_foo_1 : Inst<Foo_1>
output out_foo_2 : Inst<Foo_2>

object foo_1 of Foo_1
propassign out_foo_1, foo_1

object foo_2 of Foo_2
propassign out_foo_2, foo_2

; CHECK: om.path_create reference %basepath [[NLA1]]
propassign out_1, path("OMReferenceTarget:~Test|CPU_1>out")
; CHECK: om.path_create reference %basepath [[NLA2]]
propassign out_2, path("OMReferenceTarget:~Test|CPU_1/fetch_1:Fetch_1>foo")

; CHECK-NOT: OM_2
class OM_2 :
output out_1 : Path
output out_2 : Path
output out_foo_1 : Inst<Foo_1>
output out_foo_2 : Inst<Foo_2>

object foo_1 of Foo_1
propassign out_foo_1, foo_1

object foo_2 of Foo_2
propassign out_foo_2, foo_2

propassign out_1, path("OMReferenceTarget:~Test|CPU_2>out")
propassign out_2, path("OMReferenceTarget:~Test|CPU_2/fetch_1:Fetch_2>foo")

0 comments on commit 19d522f

Please sign in to comment.