diff --git a/lib/Dialect/FIRRTL/Transforms/Dedup.cpp b/lib/Dialect/FIRRTL/Transforms/Dedup.cpp index 080225fd48..84777ec5ba 100644 --- a/lib/Dialect/FIRRTL/Transforms/Dedup.cpp +++ b/lib/Dialect/FIRRTL/Transforms/Dedup.cpp @@ -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")); @@ -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 nonessentialAttributes; }; @@ -159,6 +163,15 @@ private: 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(result.getOwner())) { + referredModuleNames.push_back(objectOp.getType().getNameAttr().getAttr()); + return; + } + update(result.getType()); } @@ -201,8 +214,11 @@ private: 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(op) && name == constants.portNamesAttr; + if (constants.nonessentialAttributes.contains(name) && !isClassPortNames) continue; // Hash the port types. @@ -247,6 +263,11 @@ private: // 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(value)) + continue; + // If this is an symbol reference, we need to perform name erasure. if (auto innerRef = dyn_cast(value)) update(innerRef); @@ -588,6 +609,9 @@ struct Equivalence { if (failed(check(diag, a, cast(aAttr), b, cast(bAttr)))) return failure(); + } else if (isa(aAttr) && isa(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() @@ -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(); } @@ -648,8 +673,8 @@ struct Equivalence { // If its an instance operaiton, perform some checking and possibly // recurse. - if (auto aInst = dyn_cast(a)) { - auto bInst = cast(b); + if (auto aInst = dyn_cast(a)) { + auto bInst = cast(b); if (failed(check(diag, aInst, bInst))) return failure(); } @@ -940,9 +965,15 @@ private: auto *toNode = instanceGraph[toModule]; auto toModuleRef = FlatSymbolRefAttr::get(toModule.getModuleNameAttr()); for (auto *oldInstRec : llvm::make_early_inc_range(fromNode->uses())) { - auto inst = ::cast(*oldInstRec->getInstance()); - inst.setModuleNameAttr(toModuleRef); - inst.setPortNamesAttr(toModule.getPortNamesAttr()); + auto inst = oldInstRec->getInstance(); + if (auto instOp = dyn_cast(*inst)) { + instOp.setModuleNameAttr(toModuleRef); + instOp.setPortNamesAttr(toModule.getPortNamesAttr()); + } else if (auto objectOp = dyn_cast(*inst)) { + auto classLike = cast(*toNode->getModule()); + ClassType classType = detail::getInstanceTypeForClassLike(classLike); + objectOp.getResult().setType(classType); + } oldInstRec->getParent()->addInstance(inst, toNode); oldInstRec->erase(); } @@ -1523,14 +1554,8 @@ class DedupPass : public DedupBase { // 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(*module)) { + if (!module.isPrivate() || + (!module.canDiscardOnUseEmpty() && !isa(*module))) { return success(); } diff --git a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp index c0f3febf9d..747749e803 100644 --- a/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp +++ b/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp @@ -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("circt.nonlocal")) { - auto hierPathOp = + hierPathOp = dyn_cast(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(oldPath.front()).getModule(); } auto [it, inserted] = pathInfoTable.try_emplace(id); @@ -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(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(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); diff --git a/test/firtool/classes-dedupe.fir b/test/firtool/classes-dedupe.fir new file mode 100644 index 0000000000..ed4f388979 --- /dev/null +++ b/test/firtool/classes-dedupe.fir @@ -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} + 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 + output out_foo_2 : Inst + + 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 + output out_foo_2 : Inst + + 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")