From cecb3e775669039af0afb86173a0acc1836719b4 Mon Sep 17 00:00:00 2001 From: Robert Young Date: Thu, 5 Jun 2025 15:00:13 -0400 Subject: [PATCH] Inline input-only modules Add a pass which inlines input-only modules. This functionality was previously performed by extract-test-code, but when extract-test-code is disabled, we still need to perform this transformation. This transformation is notable because it ignores don't touch annotations. The code in this pass is largely a copy-paste of parts of extract-test-code. --- include/circt-c/Firtool/Firtool.h | 3 + include/circt/Dialect/HW/Passes.td | 9 + include/circt/Firtool/Firtool.h | 7 + lib/CAPI/Firtool/Firtool.cpp | 5 + lib/Dialect/SV/Transforms/CMakeLists.txt | 1 + .../Transforms/HWInlineInputOnlyModules.cpp | 319 ++++++++++++++++++ lib/Firtool/Firtool.cpp | 10 + .../Dialect/HW/inline-input-only-modules.mlir | 152 +++++++++ 8 files changed, 506 insertions(+) create mode 100644 lib/Dialect/SV/Transforms/HWInlineInputOnlyModules.cpp create mode 100644 test/Dialect/HW/inline-input-only-modules.mlir diff --git a/include/circt-c/Firtool/Firtool.h b/include/circt-c/Firtool/Firtool.h index 140adda59b5a..8d62fd8af86d 100644 --- a/include/circt-c/Firtool/Firtool.h +++ b/include/circt-c/Firtool/Firtool.h @@ -185,6 +185,9 @@ MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetEtcDisableRegisterExtraction( MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetEtcDisableModuleInlining( CirctFirtoolFirtoolOptions options, bool value); +MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetInlineInputOnlyModules( + CirctFirtoolFirtoolOptions options, bool value); + MLIR_CAPI_EXPORTED void circtFirtoolOptionsSetAddVivadoRAMAddressConflictSynthesisBugWorkaround( CirctFirtoolFirtoolOptions options, bool value); diff --git a/include/circt/Dialect/HW/Passes.td b/include/circt/Dialect/HW/Passes.td index 485bf830ef18..b49309af9eb3 100644 --- a/include/circt/Dialect/HW/Passes.td +++ b/include/circt/Dialect/HW/Passes.td @@ -92,4 +92,13 @@ def HWAggregateToComb : Pass<"hw-aggregate-to-comb", "hw::HWModuleOp"> { let dependentDialects = ["comb::CombDialect"]; } +def InlineInputOnlyModules : + Pass<"hw-inline-input-only-modules", "mlir::ModuleOp"> { + let summary = "Inline input-only modules"; + let description = [{ + This pass inlines input-only modules, which should never be emitted by + firtool. + }]; +} + #endif // CIRCT_DIALECT_HW_PASSES_TD diff --git a/include/circt/Firtool/Firtool.h b/include/circt/Firtool/Firtool.h index 537747020a5a..df0ce78f12df 100644 --- a/include/circt/Firtool/Firtool.h +++ b/include/circt/Firtool/Firtool.h @@ -114,6 +114,7 @@ class FirtoolOptions { bool shouldEtcDisableModuleInlining() const { return etcDisableModuleInlining; } + bool shouldInlineInputOnlyModules() const { return inlineInputOnlyModules; } bool shouldStripDebugInfo() const { return stripDebugInfo; } bool shouldStripFirDebugInfo() const { return stripFirDebugInfo; } bool shouldExportModuleHierarchy() const { return exportModuleHierarchy; } @@ -314,6 +315,11 @@ class FirtoolOptions { return *this; } + FirtoolOptions &setInlineInputOnlyModules(bool value) { + inlineInputOnlyModules = value; + return *this; + } + FirtoolOptions & setAddVivadoRAMAddressConflictSynthesisBugWorkaround(bool value) { addVivadoRAMAddressConflictSynthesisBugWorkaround = value; @@ -437,6 +443,7 @@ class FirtoolOptions { bool etcDisableInstanceExtraction; bool etcDisableRegisterExtraction; bool etcDisableModuleInlining; + bool inlineInputOnlyModules; bool addVivadoRAMAddressConflictSynthesisBugWorkaround; std::string ckgModuleName; std::string ckgInputName; diff --git a/lib/CAPI/Firtool/Firtool.cpp b/lib/CAPI/Firtool/Firtool.cpp index 118004fa359b..5c078992e1d9 100644 --- a/lib/CAPI/Firtool/Firtool.cpp +++ b/lib/CAPI/Firtool/Firtool.cpp @@ -271,6 +271,11 @@ void circtFirtoolOptionsSetEtcDisableModuleInlining( unwrap(options)->setEtcDisableModuleInlining(value); } +void circtFirtoolOptionsSetInlineInputOnlyModules( + CirctFirtoolFirtoolOptions options, bool value) { + unwrap(options)->setInlineInputOnlyModules(value); +} + void circtFirtoolOptionsSetAddVivadoRAMAddressConflictSynthesisBugWorkaround( CirctFirtoolFirtoolOptions options, bool value) { unwrap(options)->setAddVivadoRAMAddressConflictSynthesisBugWorkaround(value); diff --git a/lib/Dialect/SV/Transforms/CMakeLists.txt b/lib/Dialect/SV/Transforms/CMakeLists.txt index a86a4ee8cde9..0e92f1fef046 100644 --- a/lib/Dialect/SV/Transforms/CMakeLists.txt +++ b/lib/Dialect/SV/Transforms/CMakeLists.txt @@ -1,6 +1,7 @@ add_circt_dialect_library(CIRCTSVTransforms GeneratorCallout.cpp HWCleanup.cpp + HWInlineInputOnlyModules.cpp HWStubExternalModules.cpp HWLegalizeModules.cpp PrettifyVerilog.cpp diff --git a/lib/Dialect/SV/Transforms/HWInlineInputOnlyModules.cpp b/lib/Dialect/SV/Transforms/HWInlineInputOnlyModules.cpp new file mode 100644 index 000000000000..bfe6a62b2506 --- /dev/null +++ b/lib/Dialect/SV/Transforms/HWInlineInputOnlyModules.cpp @@ -0,0 +1,319 @@ +//===- HWInlineInputOnlyModules.cpp - Inline input-only HW modules --------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This transform inlines modules which do not have output ports. +// +//===----------------------------------------------------------------------===// + +#include "circt/Dialect/Emit/EmitOps.h" +#include "circt/Dialect/HW/HWAttributes.h" +#include "circt/Dialect/HW/HWInstanceGraph.h" +#include "circt/Dialect/HW/HWOps.h" +#include "circt/Dialect/HW/HWPasses.h" +#include "circt/Dialect/HW/HWSymCache.h" +#include "circt/Dialect/HW/InnerSymbolNamespace.h" +#include "circt/Dialect/SV/SVOps.h" +#include "circt/Dialect/SV/SVPasses.h" +#include "circt/Dialect/Seq/SeqDialect.h" +#include "circt/Dialect/Seq/SeqOps.h" +#include "circt/Dialect/Verif/VerifOps.h" +#include "circt/Support/Namespace.h" +#include "mlir/IR/Builders.h" +#include "mlir/IR/IRMapping.h" +#include "mlir/Pass/Pass.h" +#include "llvm/ADT/SetVector.h" + +namespace circt { +namespace hw { +#define GEN_PASS_DEF_INLINEINPUTONLYMODULES +#include "circt/Dialect/HW/Passes.h.inc" +} // namespace hw +} // namespace circt + +using namespace mlir; +using namespace circt; +using namespace sv; +using namespace hw; + +using BindTable = DenseMap>; + +// Check if the module has already been bound. +static bool isBound(hw::HWModuleLike op, hw::InstanceGraph &instanceGraph) { + auto *node = instanceGraph.lookup(op); + return llvm::any_of(node->uses(), [](igraph::InstanceRecord *a) { + auto inst = a->getInstance(); + if (!inst) + return false; + + return inst.getDoNotPrint(); + }); +} + +// Add any existing bindings to the bind table. +static void addExistingBinds(Block *topLevelModule, BindTable &bindTable) { + for (auto bind : topLevelModule->getOps()) { + hw::InnerRefAttr boundRef = bind.getInstance(); + bindTable[boundRef.getModule()][boundRef.getName()] = bind; + } +} + +// Check if op has any operand using a value that isn't yet defined. +static bool hasOOOArgs(hw::HWModuleOp newMod, Operation *op) { + for (auto arg : op->getOperands()) { + auto *argOp = arg.getDefiningOp(); // may be null + if (!argOp) + continue; + if (argOp->getParentOfType() != newMod) + return true; + } + return false; +} + +// Update any operand which was emitted before its defining op was. +static void updateOOOArgs(SmallVectorImpl &lateBoundOps, + IRMapping &cutMap) { + for (auto *op : lateBoundOps) + for (unsigned argidx = 0, e = op->getNumOperands(); argidx < e; ++argidx) { + Value arg = op->getOperand(argidx); + if (cutMap.contains(arg)) + op->setOperand(argidx, cutMap.lookup(arg)); + } +} + +// Inline any modules that only have inputs for test code. +static void +inlineInputOnly(hw::HWModuleOp oldMod, hw::InstanceGraph &instanceGraph, + BindTable &bindTable, + llvm::DenseSet &innerRefUsedByNonBindOp) { + // Check if the module only has inputs. + if (oldMod.getNumOutputPorts() != 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). + auto oldModName = oldMod.getModuleNameAttr(); + for (auto port : oldMod.getPortList()) { + auto sym = port.getSym(); + if (sym) { + for (auto property : sym) { + auto innerRef = hw::InnerRefAttr::get(oldModName, property.getName()); + if (innerRefUsedByNonBindOp.count(innerRef)) { + 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()->getOps()) { + if (auto innerSym = op.getInnerSymAttr()) { + for (auto property : innerSym) { + auto innerRef = hw::InnerRefAttr::get(oldModName, property.getName()); + if (innerRefUsedByNonBindOp.count(innerRef)) { + op.emitWarning() << "module " << oldMod.getModuleName() + << " is an input only module but cannot be inlined " + "because signals are referred by name"; + return; + } + } + } + } + + SmallPtrSet opsToErase; + + // Get the instance graph node for the old module. + igraph::InstanceGraphNode *node = instanceGraph.lookup(oldMod); + + // Iterate through each instance of the module. + OpBuilder b(oldMod); + bool allInlined = true; + for (igraph::InstanceRecord *use : llvm::make_early_inc_range(node->uses())) { + // If there is no instance, move on. + auto instLike = use->getInstance(); + if (!instLike) { + allInlined = false; + continue; + } + + // If the instance had a symbol, we can't inline it without more work. + hw::InstanceOp inst = cast(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; + } + + // Build a mapping from module block arguments to instance inputs. + IRMapping mapping; + assert(inst.getInputs().size() == oldMod.getNumInputPorts()); + auto inputPorts = oldMod.getBodyBlock()->getArguments(); + for (size_t i = 0, e = inputPorts.size(); i < e; ++i) + mapping.map(inputPorts[i], inst.getOperand(i)); + + // Inline the body at the instantiation site. + hw::HWModuleOp instParent = + cast(use->getParent()->getModule()); + igraph::InstanceGraphNode *instParentNode = + instanceGraph.lookup(instParent); + SmallVector lateBoundOps; + b.setInsertionPoint(inst); + // Namespace that tracks inner symbols in the parent module. + hw::InnerSymbolNamespace nameSpace(instParent); + // A map from old inner symbols to new ones. + DenseMap 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 innerSymOp = dyn_cast(op)) { + if (auto innerSym = innerSymOp.getInnerSymAttr()) { + for (auto property : innerSym) { + auto oldName = property.getName(); + auto newName = + b.getStringAttr(nameSpace.newName(oldName.getValue())); + auto result = symMapping.insert({oldName, 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(op)) { + if (auto innerInstSym = innerInst.getInnerSymAttr()) { + auto it = + bindTable[oldMod.getNameAttr()].find(innerInstSym.getSymName()); + if (it != bindTable[oldMod.getNameAttr()].end()) { + sv::BindOp bind = it->second; + auto oldInnerRef = bind.getInstanceAttr(); + 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(b.clone(*bind, mapping)); + clonedBind.setInstanceAttr(newInnerRef); + bindTable[instParent.getModuleNameAttr()][newName] = + cast(clonedBind); + } + } + } + + // For all ops besides the output, clone into the parent body. + if (!isa(op)) { + Operation *clonedOp = b.clone(op, mapping); + // If some of the operands haven't been cloned over yet, due to cycles, + // remember to revisit this op. + if (hasOOOArgs(instParent, clonedOp)) + lateBoundOps.push_back(clonedOp); + + // If the cloned op is an instance, record it within the new parent in + // the instance graph. + if (auto innerInst = dyn_cast(clonedOp)) { + igraph::InstanceGraphNode *innerInstModule = + instanceGraph.lookup(innerInst.getModuleNameAttr().getAttr()); + instParentNode->addInstance(innerInst, innerInstModule); + } + + // If the cloned op has an inner sym, then attach an updated inner sym. + if (auto innerSymOp = dyn_cast(clonedOp)) { + if (auto oldInnerSym = innerSymOp.getInnerSymAttr()) { + SmallVector properties; + for (auto property : oldInnerSym) { + auto newSymName = symMapping[property.getName()]; + properties.push_back(hw::InnerSymPropertiesAttr::get( + op.getContext(), newSymName, property.getFieldID(), + property.getSymVisibility())); + } + auto innerSym = hw::InnerSymAttr::get(op.getContext(), properties); + innerSymOp.setInnerSymbolAttr(innerSym); + } + } + } + } + + // Map over any ops that didn't have their operands mapped when cloned. + updateOOOArgs(lateBoundOps, mapping); + + // Erase the old instantiation site. + assert(inst.use_empty() && "inlined instance should have no uses"); + use->erase(); + opsToErase.insert(inst); + } + + // 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); + } + + while (!opsToErase.empty()) { + Operation *op = *opsToErase.begin(); + op->walk([&](Operation *erasedOp) { opsToErase.erase(erasedOp); }); + op->dropAllUses(); + op->erase(); + } +} + +namespace { +struct InlineInputOnlyModulesPass + : public circt::hw::impl::InlineInputOnlyModulesBase< + InlineInputOnlyModulesPass> { + void runOnOperation() override; +}; +} // namespace + +void InlineInputOnlyModulesPass::runOnOperation() { + auto &instanceGraph = getAnalysis(); + auto top = getOperation(); + BindTable bindTable; + addExistingBinds(top.getBody(), bindTable); + + // 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 innerRefUsedByNonBindOp; + top.walk([&](Operation *op) { + if (!isa(op)) + for (auto attr : op->getAttrs()) + attr.getValue().walk([&](hw::InnerRefAttr attr) { + innerRefUsedByNonBindOp.insert(attr); + }); + }); + + for (auto module : + llvm::make_early_inc_range(top.getBody()->getOps())) { + if (isBound(module, instanceGraph)) + continue; + + inlineInputOnly(module, instanceGraph, bindTable, innerRefUsedByNonBindOp); + } + + markAnalysesPreserved(); +} diff --git a/lib/Firtool/Firtool.cpp b/lib/Firtool/Firtool.cpp index ec54599d7579..488de912655b 100644 --- a/lib/Firtool/Firtool.cpp +++ b/lib/Firtool/Firtool.cpp @@ -330,6 +330,9 @@ LogicalResult firtool::populateHWToSV(mlir::PassManager &pm, opt.shouldEtcDisableRegisterExtraction(), opt.shouldEtcDisableModuleInlining())); + if (opt.shouldInlineInputOnlyModules()) + pm.addPass(hw::createInlineInputOnlyModules()); + pm.addPass(seq::createExternalizeClockGatePass(opt.getClockGateOptions())); pm.addPass(circt::createLowerSimToSVPass()); pm.addPass(circt::createLowerSeqToSVPass( @@ -699,6 +702,11 @@ struct FirtoolCmdOptions { llvm::cl::desc("Disable inlining modules that only feed test code"), llvm::cl::init(false)}; + llvm::cl::opt inlineInputOnlyModules{ + "inline-input-only-modules", + llvm::cl::desc("Inline input-only modules"), + llvm::cl::init(false)}; + llvm::cl::opt addVivadoRAMAddressConflictSynthesisBugWorkaround{ "add-vivado-ram-address-conflict-synthesis-bug-workaround", llvm::cl::desc( @@ -819,6 +827,7 @@ circt::firtool::FirtoolOptions::FirtoolOptions() verificationFlavor(firrtl::VerificationFlavor::None), emitSeparateAlwaysBlocks(false), etcDisableInstanceExtraction(false), etcDisableRegisterExtraction(false), etcDisableModuleInlining(false), + inlineInputOnlyModules(false), addVivadoRAMAddressConflictSynthesisBugWorkaround(false), ckgModuleName("EICG_wrapper"), ckgInputName("in"), ckgOutputName("out"), ckgEnableName("en"), ckgTestEnableName("test_en"), ckgInstName("ckg"), @@ -864,6 +873,7 @@ circt::firtool::FirtoolOptions::FirtoolOptions() etcDisableInstanceExtraction = clOptions->etcDisableInstanceExtraction; etcDisableRegisterExtraction = clOptions->etcDisableRegisterExtraction; etcDisableModuleInlining = clOptions->etcDisableModuleInlining; + inlineInputOnlyModules = clOptions->inlineInputOnlyModules; addVivadoRAMAddressConflictSynthesisBugWorkaround = clOptions->addVivadoRAMAddressConflictSynthesisBugWorkaround; ckgModuleName = clOptions->ckgModuleName; diff --git a/test/Dialect/HW/inline-input-only-modules.mlir b/test/Dialect/HW/inline-input-only-modules.mlir new file mode 100644 index 000000000000..768eb1a3afb4 --- /dev/null +++ b/test/Dialect/HW/inline-input-only-modules.mlir @@ -0,0 +1,152 @@ +// RUN: circt-opt --hw-inline-input-only-modules --split-input-file --verify-diagnostics %s | FileCheck %s + +// Check a module with no ports is empty. +module { + // CHECK-NOT: @InputOnly + hw.module private @InputOnly() {} + + hw.module @Top() { + // CHECK-NOT: @InputOnly + hw.instance "inst" @InputOnly() -> () + } +} + +// ----- + +// Check an "empty" module is inlined. +module { + // CHECK-NOT: @InputOnly + hw.module private @InputOnly(in %clock: i1, in %cond: i1) {} + + hw.module @Top(in %clock: i1, in %a: i1, in %b: i1) { + // CHECK-NOT: @InputOnly + hw.instance "inst" @InputOnly(clock: %clock: i1, cond: %b: i1) -> () + } +} + +// ----- + +// Check a module with IR in its body is inlined. +module { + // CHECK-NOT: @InputOnly + hw.module private @InputOnly(in %x: i1) { + %0 = comb.and %x : i1 + } + + hw.module @Top(in %clock: i1, in %x: i1) { + // CHECK-NOT: @InputOnly + // CHECK: comb.and %x + hw.instance "inst" @InputOnly(x: %x: i1) -> () + } +} + +// ----- + +// Check that a module with IR which is "out of order" is inlined correctly. +module { + // CHECK-NOT: @InputOnly + hw.module private @InputOnly(in %x: i1) { + %0 = comb.and %1 : i1 + %1 = comb.and %x : i1 + } + + hw.module @Top(in %clock: i1, in %x: i1) { + // CHECK-NOT: @InputOnly + // CHECK: %0 = comb.and %1 : i1 + // CHECK: %1 = comb.and %x : i1 + hw.instance "inst" @InputOnly(x: %x: i1) -> () + } +} + +// ----- + +// Check that if a module has bound-in instances, it cannot be inlined. +module { + // This module cannot be inlined because it is bound in. + // CHECK: @Bound() + hw.module private @Bound() {} + + // This module must be inlined. + // CHECK-NOT: @Component + hw.module private @Component() { + hw.instance "bound" sym @bound @Bound() -> () {doNotPrint} + } + + // CHECK: @Top + hw.module @Top() { + // CHECK: @Bound + hw.instance "component" @Component() -> () + } + + // CHECK: sv.bind <@Top::@bound> + sv.bind <@Component::@bound> +} + +// ----- + +// Check that bind statements are cloned appropriately. + +// Check that if a module has bound-in instances, it cannot be inlined. +module { + // CHECK: @Bound() + hw.module private @Bound() {} + + // This module must be inlined. + // CHECK-NOT: @Component + hw.module private @Component() { + hw.instance "bound" sym @bound @Bound() -> () {doNotPrint} + } + + // CHECK: @Top + hw.module @Top() { + // CHECK: @Bound + hw.instance "component1" @Component() -> () + // CHECK: @Bound + hw.instance "component2" @Component() -> () + } + + // This bind statement is cloned out when component1 and 2 are inlined into top. + // CHECK: sv.bind <@Top::@bound> + // CHECK: sv.bind <@Top::@bound_0> + sv.bind <@Component::@bound> +} + +// ----- + +// Check that a module cannot be inlined if an instance has an inner sym, and +// that inner sym is not used by a non-bind operation. +module { + // CHECK: @Component + // expected-warning @below {{Component cannot be inlined because there is an instance with a symbol}} + hw.module private @Component() {} + + // CHECK: @Top + hw.module @Top() { + // CHECK: "foo" sym @foo @Component + // expected-note @below {{}} + hw.instance "foo" sym @foo @Component() -> () + } +} + +// ----- + +// @ShouldNotBeInlined cannot be inlined because there is a wire with an inner +// sym, which is referred by hierpath op. +module { + hw.hierpath private @Foo [@ShouldNotBeInlined::@foo] + + hw.module private @ShouldNotBeInlined(in %clock: i1, in %a: i1) { + // expected-warning @below {{module ShouldNotBeInlined is an input only module but cannot be inlined because signals are referred by name}} + %w = sv.wire sym @foo: !hw.inout + sv.always posedge %clock { + sv.if %a { + sv.assert %a, immediate message "foo" + } + } + hw.output + } + + hw.module @Top(in %clock: i1, in %a: i1) { + hw.instance "foo" @ShouldNotBeInlined(clock: %clock: i1, a: %a: i1) -> () + } +}