From 85c55f905e5d7174b34974d0ee421a424027326c Mon Sep 17 00:00:00 2001 From: Hideto Ueno Date: Wed, 20 May 2026 00:12:45 -0700 Subject: [PATCH 1/4] [FIRRTL] Replace remove-unused-ports with IMDCE mode --- include/circt/Dialect/FIRRTL/Passes.td | 22 +- lib/Dialect/FIRRTL/FIRRTLReductions.cpp | 15 +- lib/Dialect/FIRRTL/Transforms/CMakeLists.txt | 1 - .../FIRRTL/Transforms/IMDeadCodeElim.cpp | 96 +++++--- .../FIRRTL/Transforms/RemoveUnusedPorts.cpp | 221 ------------------ .../Reduction/pattern-registration.mlir | 2 +- .../Dialect/FIRRTL/Reduction/port-pruner.mlir | 14 +- test/Dialect/FIRRTL/remove-unused-ports.mlir | 110 ++++++--- 8 files changed, 167 insertions(+), 314 deletions(-) delete mode 100644 lib/Dialect/FIRRTL/Transforms/RemoveUnusedPorts.cpp diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 744758656113..37b211d15fef 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -124,22 +124,6 @@ def RegisterOptimizer : Pass<"firrtl-register-optimizer", "firrtl::FModuleOp"> { }]; } -def RemoveUnusedPorts : Pass<"firrtl-remove-unused-ports", "firrtl::CircuitOp"> { - let summary = "Remove unused ports"; - let description = [{ - This pass removes unused ports without annotations or symbols. Implementation - wise, this pass iterates over the instance graph in a topological order from - leaves to the top so that we can remove unused ports optimally. - }]; - let options = [ - Option<"ignoreDontTouch", "ignore-dont-touch", "bool", "false", - "remove unused ports even if they have a symbol or annotation"> - ]; - let statistics = [ - Statistic<"numRemovedPorts", "num-removed-ports", "Number of ports erased">, - ]; -} - def IMDeadCodeElim : Pass<"firrtl-imdeadcodeelim", "mlir::ModuleOp"> { let summary = "Intermodule dead code elimination"; let description = [{ @@ -148,6 +132,12 @@ def IMDeadCodeElim : Pass<"firrtl-imdeadcodeelim", "mlir::ModuleOp"> { of public modules or a value with a symbol. We first populate alive values into a set, and then propagate the liveness by looking at their dataflow. }]; + let options = [ + Option<"removePortsOnly", "remove-ports-only", "bool", "false", + "only remove dead module ports and preserve other operations">, + Option<"ignoreDontTouch", "ignore-dont-touch", "bool", "false", + "remove unused ports even if they have a symbol or annotation"> + ]; let statistics = [ Statistic<"numErasedOps", "num-erased-ops", "Number of operations erased">, Statistic<"numErasedModules", "num-erased-modules", "Number of modules erased">, diff --git a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp index c7cb08cbbea8..e2a79b9d866e 100644 --- a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp @@ -2663,6 +2663,17 @@ struct ExtmoduleConventionRemover : public OpReduction { bool isOneShot() const override { return true; } }; +struct IMDCEPortReduction : public PassReduction { + IMDCEPortReduction(MLIRContext *context) + : PassReduction( + context, firrtl::createIMDeadCodeElim({/*removePortsOnly=*/true, + /*ignoreDontTouch=*/true})) { + } + std::string getName() const override { + return "firrtl-imdeadcodeelim-remove-ports"; + } +}; + //===----------------------------------------------------------------------===// // Reduction Registration //===----------------------------------------------------------------------===// @@ -2702,9 +2713,7 @@ void firrtl::FIRRTLReducePatternDialectInterface::populateReducePatterns( true, true); patterns.add(getContext(), firrtl::createInliner()); patterns.add(getContext(), firrtl::createIMConstProp()); - patterns.add( - getContext(), - firrtl::createRemoveUnusedPorts({/*ignoreDontTouch=*/true})); + patterns.add(getContext()); patterns.add(); patterns.add(getContext(), firrtl::createIMDeadCodeElim()); patterns.add(); diff --git a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt index 70b8e3f780f0..9dd930292a48 100755 --- a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt +++ b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt @@ -56,7 +56,6 @@ add_circt_dialect_library(CIRCTFIRRTLTransforms ProbesToSignals.cpp RandomizeRegisterInit.cpp RegisterOptimizer.cpp - RemoveUnusedPorts.cpp ResolvePaths.cpp ResolveTraces.cpp SFCCompat.cpp diff --git a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp index a396e4a65e4b..d4c3dd3d5d11 100644 --- a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp +++ b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp @@ -56,6 +56,8 @@ static bool isDeletableDeclaration(Operation *op) { namespace { struct IMDeadCodeElimPass : public circt::firrtl::impl::IMDeadCodeElimBase { + using Base::Base; + void runOnOperation() override; void rewriteModuleSignature(FModuleOp module); @@ -81,6 +83,12 @@ struct IMDeadCodeElimPass return executableBlocks.count(block); } + /// Return true if the value has a don't touch annotation that should be + /// preserved (respecting the ignoreDontTouch option). + bool shouldPreserveDontTouch(Value value) const { + return !ignoreDontTouch && firrtl::hasDontTouch(value); + } + void visitUser(Operation *op); void visitValue(Value value); @@ -200,6 +208,12 @@ void IMDeadCodeElimPass::markUnknownSideEffectOp(Operation *op) { for (auto operand : op->getOperands()) markAlive(operand); markBlockUndeletable(op); + + // Recursively mark any blocks contained within these operations as + // executable. + for (auto ®ion : op->getRegions()) + for (auto &block : region.getBlocks()) + markBlockExecutable(&block); } void IMDeadCodeElimPass::visitUser(Operation *op) { @@ -266,41 +280,48 @@ void IMDeadCodeElimPass::markBlockExecutable(Block *block) { return; // Already executable. auto fmodule = dyn_cast(block->getParentOp()); - if (fmodule && fmodule.isPublic()) + if (fmodule && (fmodule.isPublic() || removePortsOnly)) markAlive(fmodule); // Mark ports with don't touch as alive. for (auto blockArg : block->getArguments()) - if (hasDontTouch(blockArg)) { + if (shouldPreserveDontTouch(blockArg)) { markAlive(blockArg); if (fmodule) markAlive(fmodule); } for (auto &op : *block) { - if (isDeclaration(&op)) - markDeclaration(&op); - else if (auto instance = dyn_cast(op)) + if (auto instance = dyn_cast(op)) { markFInstanceLikeOp(instance); - else if (isa(op)) - // Skip connect op. continue; - else if (hasUnknownSideEffect(&op)) { + } + + // Skip connects in both modes. + if (isa(op)) + continue; + + if (removePortsOnly) { + // In port-only mode, all non-instance, non-connect ops are alive. markUnknownSideEffectOp(&op); - // Recursively mark any blocks contained within these operations as - // executable. - for (auto ®ion : op.getRegions()) - for (auto &block : region.getBlocks()) - markBlockExecutable(&block); + continue; } + // Full IMDCE mode: handle declarations and side effects. + if (isDeclaration(&op)) + markDeclaration(&op); + else if (hasUnknownSideEffect(&op)) + markUnknownSideEffectOp(&op); + // TODO: Handle attach etc. } } void IMDeadCodeElimPass::forwardConstantOutputPort(FModuleOp module) { - // This tracks constant values of output ports. - SmallVector> constantPortIndicesAndValues; + // This tracks constant values of output ports. std::nullopt represents an + // invalid value. + SmallVector>> + constantPortIndicesAndValues; auto ports = module.getPorts(); auto *instanceGraphNode = instanceGraph->lookup(module); @@ -310,16 +331,19 @@ void IMDeadCodeElimPass::forwardConstantOutputPort(FModuleOp module) { auto arg = module.getArgument(index); // If the port has don't touch, don't propagate the constant value. - if (!port.isOutput() || hasDontTouch(arg)) + if (!port.isOutput() || shouldPreserveDontTouch(arg)) continue; // Remember the index and constant value connected to an output port. - if (auto connect = getSingleConnectUserOf(arg)) + if (auto connect = getSingleConnectUserOf(arg)) { if (auto constant = connect.getSrc().getDefiningOp()) constantPortIndicesAndValues.push_back({index, constant.getValue()}); + else if (connect.getSrc().getDefiningOp()) + constantPortIndicesAndValues.push_back({index, std::nullopt}); + } } - // If there is no constant port, abort. + // If there is no constant or invalid port, abort. if (constantPortIndicesAndValues.empty()) return; @@ -334,8 +358,13 @@ void IMDeadCodeElimPass::forwardConstantOutputPort(FModuleOp module) { auto result = instance.getResult(index); assert(ports[index].isOutput() && "must be an output port"); - // Replace the port with the constant. - result.replaceAllUsesWith(ConstantOp::create(builder, constant)); + // Replace the port with the constant or invalid value. + Value replacement; + if (constant) + replacement = ConstantOp::create(builder, *constant); + else + replacement = InvalidValueOp::create(builder, result.getType()); + result.replaceAllUsesWith(replacement); } } } @@ -435,11 +464,13 @@ void IMDeadCodeElimPass::runOnOperation() { forwardConstantOutputPort(module); for (auto module : circuit.getBodyBlock()->getOps()) { - // Mark the ports of public modules as alive. - if (module.isPublic()) { + bool isPublic = module.isPublic(); + if (isPublic || removePortsOnly) { markBlockExecutable(module.getBodyBlock()); - for (auto port : module.getBodyBlock()->getArguments()) - markAlive(port); + // Mark the ports of public modules as alive. + if (isPublic) + for (auto port : module.getBodyBlock()->getArguments()) + markAlive(port); } // Walk annotations and populate a map from hierpath to attached annotation @@ -459,9 +490,11 @@ void IMDeadCodeElimPass::runOnOperation() { return false; }; - AnnotationSet::removePortAnnotations(module, visitAnnotation); - AnnotationSet::removeAnnotations( - module, std::bind(visitAnnotation, -1, std::placeholders::_1)); + if (!ignoreDontTouch) { + AnnotationSet::removePortAnnotations(module, visitAnnotation); + AnnotationSet::removeAnnotations( + module, std::bind(visitAnnotation, -1, std::placeholders::_1)); + } } // If an element changed liveness then propagate liveness through it. @@ -502,8 +535,9 @@ void IMDeadCodeElimPass::runOnOperation() { if (!liveElements.count(op)) op.erase(); - for (auto module : modules) - eraseEmptyModule(module); + if (!removePortsOnly) + for (auto module : modules) + eraseEmptyModule(module); // Clean up data structures. executableBlocks.clear(); @@ -710,11 +744,11 @@ void IMDeadCodeElimPass::rewriteModuleSignature(FModuleOp module) { for (auto index : llvm::seq(0u, numOldPorts)) { auto argument = module.getArgument(index); - assert((!hasDontTouch(argument) || isKnownAlive(argument)) && + assert((!shouldPreserveDontTouch(argument) || isKnownAlive(argument)) && "If the port has don't touch, it should be known alive"); // If the port has dontTouch, skip. - if (hasDontTouch(argument)) + if (shouldPreserveDontTouch(argument)) continue; if (isKnownAlive(argument)) { diff --git a/lib/Dialect/FIRRTL/Transforms/RemoveUnusedPorts.cpp b/lib/Dialect/FIRRTL/Transforms/RemoveUnusedPorts.cpp deleted file mode 100644 index d98d48f18895..000000000000 --- a/lib/Dialect/FIRRTL/Transforms/RemoveUnusedPorts.cpp +++ /dev/null @@ -1,221 +0,0 @@ -//===- RemoveUnusedPorts.cpp - Remove Dead Ports ----------------*- C++ -*-===// -// -// 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 -// -//===----------------------------------------------------------------------===// - -#include "circt/Dialect/FIRRTL/FIRRTLAnnotations.h" -#include "circt/Dialect/FIRRTL/FIRRTLInstanceGraph.h" -#include "circt/Dialect/FIRRTL/FIRRTLOps.h" -#include "circt/Dialect/FIRRTL/FIRRTLUtils.h" -#include "circt/Dialect/FIRRTL/Passes.h" -#include "circt/Support/Debug.h" -#include "mlir/IR/ImplicitLocOpBuilder.h" -#include "mlir/Pass/Pass.h" -#include "llvm/ADT/BitVector.h" -#include "llvm/ADT/PostOrderIterator.h" -#include "llvm/Support/Debug.h" - -#define DEBUG_TYPE "firrtl-remove-unused-ports" - -namespace circt { -namespace firrtl { -#define GEN_PASS_DEF_REMOVEUNUSEDPORTS -#include "circt/Dialect/FIRRTL/Passes.h.inc" -} // namespace firrtl -} // namespace circt - -using namespace circt; -using namespace firrtl; - -namespace { - -struct RemoveUnusedPortsPass - : public circt::firrtl::impl::RemoveUnusedPortsBase { - using Base::Base; - - void runOnOperation() override; - void removeUnusedModulePorts(FModuleOp module, - InstanceGraphNode *instanceGraphNode); - - /// If true, the pass will remove unused ports even if they have carry a - /// symbol or annotations. This is likely to break the IR, but may be useful - /// for `circt-reduce` where preserving functional correctness of the IR is - /// not important. - bool ignoreDontTouch = false; -}; -} // namespace - -void RemoveUnusedPortsPass::runOnOperation() { - CIRCT_DEBUG_SCOPED_PASS_LOGGER(this); - - auto &instanceGraph = getAnalysis(); - // Iterate in the reverse order of instance graph iterator, i.e. from leaves - // to top. - for (auto *node : llvm::post_order(&instanceGraph)) - if (auto module = dyn_cast(*node->getModule())) - // Don't prune the main module. - if (!module.isPublic()) - removeUnusedModulePorts(module, node); -} - -void RemoveUnusedPortsPass::removeUnusedModulePorts( - FModuleOp module, InstanceGraphNode *instanceGraphNode) { - LLVM_DEBUG(llvm::dbgs() << "Prune ports of module: " << module.getName() - << "\n"); - // This tracks constant values of output ports. None indicates an invalid - // value. - SmallVector> outputPortConstants; - auto ports = module.getPorts(); - // This tracks port indexes that can be erased. - llvm::BitVector removalPortIndexes(ports.size()); - - for (const auto &e : llvm::enumerate(ports)) { - unsigned index = e.index(); - auto port = e.value(); - auto arg = module.getArgument(index); - - // If the port is don't touch or has unprocessed annotations, we cannot - // remove the port. - if ((hasDontTouch(arg) || !port.annotations.empty()) && !ignoreDontTouch) - continue; - - // TODO: Handle inout ports. - if (port.isInOut()) - continue; - - // If the port is input and has an user, we cannot remove the - // port. - if (port.isInput() && !arg.use_empty()) - continue; - - auto portIsUnused = [&](InstanceRecord *a) -> bool { - auto port = a->getInstance()->getResult(arg.getArgNumber()); - return port.getUses().empty(); - }; - - // Output port. - if (port.isOutput()) { - if (arg.use_empty()) { - // Sometimes the connection is already removed possibly by IMCP. - // In that case, regard the port value as an invalid value. - outputPortConstants.push_back(std::nullopt); - } else if (llvm::all_of(instanceGraphNode->uses(), portIsUnused)) { - // Replace the port with a wire if it is unused. - auto builder = ImplicitLocOpBuilder::atBlockBegin( - arg.getLoc(), module.getBodyBlock()); - auto wire = WireOp::create(builder, arg.getType()); - arg.replaceAllUsesWith(wire.getResult()); - outputPortConstants.push_back(std::nullopt); - } else if (arg.hasOneUse()) { - // If the port has a single use, check the port is only connected to - // invalid or constant - Operation *op = arg.use_begin().getUser(); - auto connectLike = dyn_cast(op); - if (!connectLike) - continue; - auto *srcOp = connectLike.getSrc().getDefiningOp(); - if (!isa_and_nonnull(srcOp)) - continue; - - if (auto constant = dyn_cast(srcOp)) - outputPortConstants.push_back(constant.getValue()); - else { - assert(isa(srcOp) && "only expect invalid"); - outputPortConstants.push_back(std::nullopt); - } - - // Erase connect op because we are going to remove this output ports. - op->erase(); - - if (srcOp->use_empty()) - srcOp->erase(); - } else { - // Otherwise, we cannot remove the port. - continue; - } - } - - removalPortIndexes.set(index); - } - - // If there is nothing to remove, abort. - if (removalPortIndexes.none()) - return; - - // Delete ports from the module. - module.erasePorts(removalPortIndexes); - LLVM_DEBUG(llvm::for_each(removalPortIndexes.set_bits(), [&](unsigned index) { - llvm::dbgs() << "Delete port: " << ports[index].name << "\n"; - });); - - // Rewrite all uses. - for (auto *use : instanceGraphNode->uses()) { - auto instance = ::cast(*use->getInstance()); - ImplicitLocOpBuilder builder(instance.getLoc(), instance); - TieOffCache tieOffCache(builder); - unsigned outputPortIndex = 0; - for (auto index : removalPortIndexes.set_bits()) { - auto result = instance.getResult(index); - assert(!ports[index].isInOut() && "don't expect inout ports"); - - // If the port is input, replace the port with an unwritten wire - // so that we can remove use-chains in SV dialect canonicalization. - if (ports[index].isInput()) { - WireOp wire = WireOp::create(builder, result.getType()); - - // Check that the input port is only written. Sometimes input ports are - // used as temporary wires. In that case, we cannot erase connections. - bool onlyWritten = llvm::all_of(result.getUsers(), [&](Operation *op) { - if (auto connect = dyn_cast(op)) - return connect.getDest() == result; - return false; - }); - - result.replaceUsesWithIf(wire.getResult(), [&](OpOperand &op) -> bool { - // Connects can be deleted directly. - if (onlyWritten && isa(op.getOwner())) { - op.getOwner()->erase(); - return false; - } - return true; - }); - - // If the wire doesn't have an user, just erase it. - if (wire.use_empty()) - wire.erase(); - - continue; - } - - // Output port. Replace with the output port with an invalid or constant - // value. - auto portConstant = outputPortConstants[outputPortIndex++]; - Value value; - if (portConstant) - value = ConstantOp::create(builder, *portConstant); - else { - // Use TieOffCache to create tie-off values for output ports. - if (auto baseType = - dyn_cast(result.getType())) { - value = tieOffCache.getInvalid(baseType); - } else { - // For non-base types like ref types, we cannot create an invalid - // value. Skip replacing uses for these types. - continue; - } - } - - result.replaceAllUsesWith(value); - } - - // Create a new instance op without unused ports. - instance.cloneWithErasedPortsAndReplaceUses(removalPortIndexes); - // Remove old one. - instance.erase(); - } - - numRemovedPorts += removalPortIndexes.count(); -} diff --git a/test/Dialect/FIRRTL/Reduction/pattern-registration.mlir b/test/Dialect/FIRRTL/Reduction/pattern-registration.mlir index 46e2eb5fd5f1..483b21508b85 100644 --- a/test/Dialect/FIRRTL/Reduction/pattern-registration.mlir +++ b/test/Dialect/FIRRTL/Reduction/pattern-registration.mlir @@ -33,7 +33,7 @@ // CHECK-DAG: firrtl-operand0-forwarder // CHECK-DAG: firrtl-operand1-forwarder // CHECK-DAG: firrtl-operand2-forwarder -// CHECK-DAG: firrtl-remove-unused-ports +// CHECK-DAG: firrtl-imdeadcodeelim-remove-ports // CHECK-DAG: hw-constantifier // CHECK-DAG: hw-module-externalizer // CHECK-DAG: hw-module-internal-name-sanitizer diff --git a/test/Dialect/FIRRTL/Reduction/port-pruner.mlir b/test/Dialect/FIRRTL/Reduction/port-pruner.mlir index ca699d8201f8..f2e8317968e8 100644 --- a/test/Dialect/FIRRTL/Reduction/port-pruner.mlir +++ b/test/Dialect/FIRRTL/Reduction/port-pruner.mlir @@ -1,6 +1,6 @@ // UNSUPPORTED: system-windows // See https://github.com/llvm/circt/issues/4129 -// RUN: circt-reduce %s --test /usr/bin/env --test-arg grep --test-arg -q --test-arg "firrtl.module private @Bar" --keep-best=0 --include firrtl-remove-unused-ports | FileCheck %s +// RUN: circt-reduce %s --test /usr/bin/env --test-arg grep --test-arg -q --test-arg "firrtl.module private @Bar" --keep-best=0 --include firrtl-imdeadcodeelim-remove-ports | FileCheck %s firrtl.circuit "Foo" { // CHECK-LABEL: firrtl.module @Foo @@ -10,11 +10,11 @@ firrtl.circuit "Foo" { // CHECK-NOT: %bar_e // CHECK: %bar_b, %bar_d = firrtl.instance bar @Bar %bar_a, %bar_b, %bar_c, %bar_d, %bar_e = firrtl.instance bar @Bar (in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>, out d: !firrtl.uint<1>, out e: !firrtl.uint<1>) - firrtl.connect %bar_a, %x : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %bar_b, %x : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %bar_a, %x : !firrtl.uint<1> + firrtl.matchingconnect %bar_b, %x : !firrtl.uint<1> %0 = firrtl.add %bar_c, %bar_d : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<2> %1 = firrtl.add %0, %bar_e : (!firrtl.uint<2>, !firrtl.uint<1>) -> !firrtl.uint<3> - firrtl.connect %y, %1 : !firrtl.uint<3>, !firrtl.uint<3> + firrtl.matchingconnect %y, %1 : !firrtl.uint<3> } // We're only ever using ports %b and %d -- the rest should be stripped. @@ -33,8 +33,8 @@ firrtl.circuit "Foo" { ) { %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1> %0 = firrtl.not %b : (!firrtl.uint<1>) -> !firrtl.uint<1> - firrtl.connect %c, %invalid_ui1 : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d, %0 : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %e, %invalid_ui1 : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %c, %invalid_ui1 : !firrtl.uint<1> + firrtl.matchingconnect %d, %0 : !firrtl.uint<1> + firrtl.matchingconnect %e, %invalid_ui1 : !firrtl.uint<1> } } diff --git a/test/Dialect/FIRRTL/remove-unused-ports.mlir b/test/Dialect/FIRRTL/remove-unused-ports.mlir index 6e213f41762d..5b338239a3d1 100644 --- a/test/Dialect/FIRRTL/remove-unused-ports.mlir +++ b/test/Dialect/FIRRTL/remove-unused-ports.mlir @@ -1,4 +1,5 @@ -// RUN: circt-opt -pass-pipeline='builtin.module(firrtl.circuit(firrtl-remove-unused-ports))' %s -split-input-file | FileCheck %s +// RUN: circt-opt -pass-pipeline='builtin.module(firrtl-imdeadcodeelim{remove-ports-only})' %s -split-input-file | FileCheck %s +// RUN: circt-opt -pass-pipeline='builtin.module(firrtl-imdeadcodeelim{remove-ports-only ignore-dont-touch})' %s -split-input-file | FileCheck %s --check-prefix=IGNORE firrtl.circuit "Top" { // CHECK-LABEL: firrtl.module @Top(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, // CHECK-SAME: out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) @@ -6,31 +7,33 @@ firrtl.circuit "Top" { out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) { %A_a, %A_b, %A_c, %A_d_unused, %A_d_invalid, %A_d_constant = firrtl.instance A @UseBar(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>, out d_unused: !firrtl.uint<1>, out d_invalid: !firrtl.uint<1>, out d_constant: !firrtl.uint<1>) // CHECK: %A_b, %A_c = firrtl.instance A @UseBar(in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) - // CHECK-NEXT: firrtl.connect %A_b, %b - // CHECK-NEXT: firrtl.connect %c, %A_c - // CHECK-NEXT: firrtl.connect %d_unused, %{{invalid_ui1.*}} - // CHECK-NEXT: firrtl.connect %d_invalid, %{{invalid_ui1.*}} - // CHECK-NEXT: firrtl.connect %d_constant, %{{c1_ui1.*}} - firrtl.connect %A_a, %a : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %A_b, %b : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %c, %A_c : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_unused, %A_d_unused : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_invalid, %A_d_invalid : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_constant, %A_d_constant : !firrtl.uint<1>, !firrtl.uint<1> + // CHECK-NEXT: firrtl.matchingconnect %A_b, %b + // CHECK-NEXT: firrtl.matchingconnect %c, %A_c + // CHECK-NEXT: firrtl.matchingconnect %d_unused, %{{invalid_ui1.*}} + // CHECK-NEXT: firrtl.matchingconnect %d_invalid, %{{invalid_ui1.*}} + // CHECK-NEXT: firrtl.matchingconnect %d_constant, %{{c1_ui1.*}} + firrtl.matchingconnect %A_a, %a : !firrtl.uint<1> + firrtl.matchingconnect %A_b, %b : !firrtl.uint<1> + firrtl.matchingconnect %c, %A_c : !firrtl.uint<1> + firrtl.matchingconnect %d_unused, %A_d_unused : !firrtl.uint<1> + firrtl.matchingconnect %d_invalid, %A_d_invalid : !firrtl.uint<1> + firrtl.matchingconnect %d_constant, %A_d_constant : !firrtl.uint<1> } // Check that %a, %d_unused, %d_invalid and %d_constant are removed. // CHECK-LABEL: firrtl.module private @Bar(in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) - // CHECK-NEXT: firrtl.connect %c, %b - // CHECK-NEXT: } + // CHECK-NEXT: firrtl.matchingconnect %c, %b + // CHECK: } firrtl.module private @Bar(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) { - firrtl.connect %c, %b : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %c, %b : !firrtl.uint<1> %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1> - firrtl.connect %d_invalid, %invalid_ui1 : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %d_unused, %invalid_ui1 : !firrtl.uint<1> + %invalid_ui1_2 = firrtl.invalidvalue : !firrtl.uint<1> + firrtl.matchingconnect %d_invalid, %invalid_ui1_2 : !firrtl.uint<1> %c1_i1 = firrtl.constant 1 : !firrtl.uint<1> - firrtl.connect %d_constant, %c1_i1 : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %d_constant, %c1_i1 : !firrtl.uint<1> } // Check that %a, %d_unused, %d_invalid and %d_constant are removed. @@ -38,13 +41,13 @@ firrtl.circuit "Top" { firrtl.module private @UseBar(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) { %A_a, %A_b, %A_c, %A_d_unused, %A_d_invalid, %A_d_constant = firrtl.instance A @Bar(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>, out d_unused: !firrtl.uint<1>, out d_invalid: !firrtl.uint<1>, out d_constant: !firrtl.uint<1>) - firrtl.connect %A_a, %a : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %A_a, %a : !firrtl.uint<1> // CHECK: %A_b, %A_c = firrtl.instance A @Bar(in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) - firrtl.connect %A_b, %b : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %c, %A_c : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_unused, %A_d_unused : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_invalid, %A_d_invalid : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %d_constant, %A_d_constant : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %A_b, %b : !firrtl.uint<1> + firrtl.matchingconnect %c, %A_c : !firrtl.uint<1> + firrtl.matchingconnect %d_unused, %A_d_unused : !firrtl.uint<1> + firrtl.matchingconnect %d_invalid, %A_d_invalid : !firrtl.uint<1> + firrtl.matchingconnect %d_constant, %A_d_constant : !firrtl.uint<1> } // Make sure that %a, %b and %c are not erased because they have an annotation or a symbol. @@ -52,18 +55,18 @@ firrtl.circuit "Top" { firrtl.module private @Foo(in %a: !firrtl.uint<1> sym @dntSym, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1> sym @dntSym2) attributes { portAnnotations = [[], [{a = "a"}], []]} { - // CHECK: firrtl.connect %c, %{{invalid_ui1.*}} + // CHECK: firrtl.matchingconnect %c, %{{invalid_ui1.*}} %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1> - firrtl.connect %c, %invalid_ui1 : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %c, %invalid_ui1 : !firrtl.uint<1> } - // CHECK-LABEL: firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) + // CHECK-LABEL: firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>) firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) { %A_a, %A_b, %A_c = firrtl.instance A @Foo(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) // CHECK: %A_a, %A_b, %A_c = firrtl.instance A @Foo(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) - firrtl.connect %A_a, %a : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %A_b, %b : !firrtl.uint<1>, !firrtl.uint<1> - firrtl.connect %c, %A_c : !firrtl.uint<1>, !firrtl.uint<1> + firrtl.matchingconnect %A_a, %a : !firrtl.uint<1> + firrtl.matchingconnect %A_b, %b : !firrtl.uint<1> + firrtl.matchingconnect %c, %A_c : !firrtl.uint<1> } } @@ -93,13 +96,15 @@ firrtl.circuit "Top" { // Check that %a, %d_unused, %d_invalid and %d_constant are removed. // CHECK-LABEL: firrtl.module private @Bar(in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) // CHECK-NEXT: firrtl.matchingconnect %c, %b - // CHECK-NEXT: } + // CHECK: } firrtl.module private @Bar(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) { firrtl.matchingconnect %c, %b : !firrtl.uint<1> %invalid_ui1 = firrtl.invalidvalue : !firrtl.uint<1> - firrtl.matchingconnect %d_invalid, %invalid_ui1 : !firrtl.uint<1> + firrtl.matchingconnect %d_unused, %invalid_ui1 : !firrtl.uint<1> + %invalid_ui1_2 = firrtl.invalidvalue : !firrtl.uint<1> + firrtl.matchingconnect %d_invalid, %invalid_ui1_2 : !firrtl.uint<1> %c1_i1 = firrtl.constant 1 : !firrtl.uint<1> firrtl.matchingconnect %d_constant, %c1_i1 : !firrtl.uint<1> } @@ -128,7 +133,7 @@ firrtl.circuit "Top" { firrtl.matchingconnect %c, %invalid_ui1 : !firrtl.uint<1> } - // CHECK-LABEL: firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) + // CHECK-LABEL: firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>) firrtl.module private @UseFoo(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>) { %A_a, %A_b, %A_c = firrtl.instance A @Foo(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) // CHECK: %A_a, %A_b, %A_c = firrtl.instance A @Foo(in a: !firrtl.uint<1>, in b: !firrtl.uint<1>, out c: !firrtl.uint<1>) @@ -140,9 +145,9 @@ firrtl.circuit "Top" { // ----- -// Ensure that the "output_file" attribute isn't destroyed by RemoveUnusedPorts. +// Ensure that the "output_file" attribute isn't destroyed by IMDCE port-only mode. // This matters for interactions between Grand Central (which sets these) and -// RemoveUnusedPorts which may clone modules with stripped ports. +// IMDCE port-only mode which may clone modules with stripped ports. // // CHECK-LABEL: "PreserveOutputFile" firrtl.circuit "PreserveOutputFile" { @@ -198,3 +203,40 @@ firrtl.circuit "ProbeTypes" { firrtl.module private @Bar(out %ref: !firrtl.probe) { } } + +// ----- + +// Test ignoreDontTouch option - removes unused ports even with symbols/annotations + +// IGNORE-LABEL: firrtl.circuit "IgnoreDontTouch" +firrtl.circuit "IgnoreDontTouch" { + firrtl.module @IgnoreDontTouch() { + // IGNORE: firrtl.instance bar @Bar() + %bar_sym, %bar_anno = firrtl.instance bar @Bar(out sym: !firrtl.uint<1>, out anno: !firrtl.uint<1>) + } + + // IGNORE-LABEL: firrtl.module private @Bar() { + // IGNORE-NEXT: } + firrtl.module private @Bar(out %sym: !firrtl.uint<1> sym @sym, out %anno: !firrtl.uint<1>) attributes { + portAnnotations = [[], [{a = "a"}]] + } { + } +} + +// ----- + +// Test case for issue #10504 - property types with instances should not crash +// This tests that unused property ports can be removed without assertion failures +firrtl.circuit "PropertyTypes" { + // CHECK-LABEL: firrtl.module @PropertyTypes + firrtl.module @PropertyTypes() { + // CHECK: firrtl.instance Bar @Bar() + %Bar_a = firrtl.instance Bar @Bar(out a: !firrtl.list) + } + + // The unused property output port should be removed without crashing. + // CHECK-LABEL: firrtl.module private @Bar() { + // CHECK-NEXT: } + firrtl.module private @Bar(out %a: !firrtl.list) { + } +} From 4988f5a0ef153540a4f596351e57410442acbb09 Mon Sep 17 00:00:00 2001 From: Hideto Ueno Date: Tue, 2 Jun 2026 16:57:28 -0700 Subject: [PATCH 2/4] Drop ignore dontTouch option --- include/circt/Dialect/FIRRTL/Passes.td | 4 +--- lib/Dialect/FIRRTL/FIRRTLReductions.cpp | 5 ++--- .../FIRRTL/Transforms/IMDeadCodeElim.cpp | 22 ++++++------------- ...rt-pruner.mlir => imdce-remove-ports.mlir} | 0 ...sed-ports.mlir => imdce-remove-ports.mlir} | 20 ----------------- 5 files changed, 10 insertions(+), 41 deletions(-) rename test/Dialect/FIRRTL/Reduction/{port-pruner.mlir => imdce-remove-ports.mlir} (100%) rename test/Dialect/FIRRTL/{remove-unused-ports.mlir => imdce-remove-ports.mlir} (94%) diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 37b211d15fef..1ca66de0590a 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -134,9 +134,7 @@ def IMDeadCodeElim : Pass<"firrtl-imdeadcodeelim", "mlir::ModuleOp"> { }]; let options = [ Option<"removePortsOnly", "remove-ports-only", "bool", "false", - "only remove dead module ports and preserve other operations">, - Option<"ignoreDontTouch", "ignore-dont-touch", "bool", "false", - "remove unused ports even if they have a symbol or annotation"> + "only remove dead module ports and preserve other operations"> ]; let statistics = [ Statistic<"numErasedOps", "num-erased-ops", "Number of operations erased">, diff --git a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp index e2a79b9d866e..3627fd36f3a7 100644 --- a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp @@ -2665,9 +2665,8 @@ struct ExtmoduleConventionRemover : public OpReduction { struct IMDCEPortReduction : public PassReduction { IMDCEPortReduction(MLIRContext *context) - : PassReduction( - context, firrtl::createIMDeadCodeElim({/*removePortsOnly=*/true, - /*ignoreDontTouch=*/true})) { + : PassReduction(context, + firrtl::createIMDeadCodeElim({/*removePortsOnly=*/true})) { } std::string getName() const override { return "firrtl-imdeadcodeelim-remove-ports"; diff --git a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp index d4c3dd3d5d11..0e0fdcc14fa8 100644 --- a/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp +++ b/lib/Dialect/FIRRTL/Transforms/IMDeadCodeElim.cpp @@ -83,12 +83,6 @@ struct IMDeadCodeElimPass return executableBlocks.count(block); } - /// Return true if the value has a don't touch annotation that should be - /// preserved (respecting the ignoreDontTouch option). - bool shouldPreserveDontTouch(Value value) const { - return !ignoreDontTouch && firrtl::hasDontTouch(value); - } - void visitUser(Operation *op); void visitValue(Value value); @@ -285,7 +279,7 @@ void IMDeadCodeElimPass::markBlockExecutable(Block *block) { // Mark ports with don't touch as alive. for (auto blockArg : block->getArguments()) - if (shouldPreserveDontTouch(blockArg)) { + if (hasDontTouch(blockArg)) { markAlive(blockArg); if (fmodule) markAlive(fmodule); @@ -331,7 +325,7 @@ void IMDeadCodeElimPass::forwardConstantOutputPort(FModuleOp module) { auto arg = module.getArgument(index); // If the port has don't touch, don't propagate the constant value. - if (!port.isOutput() || shouldPreserveDontTouch(arg)) + if (!port.isOutput() || hasDontTouch(arg)) continue; // Remember the index and constant value connected to an output port. @@ -490,11 +484,9 @@ void IMDeadCodeElimPass::runOnOperation() { return false; }; - if (!ignoreDontTouch) { - AnnotationSet::removePortAnnotations(module, visitAnnotation); - AnnotationSet::removeAnnotations( - module, std::bind(visitAnnotation, -1, std::placeholders::_1)); - } + AnnotationSet::removePortAnnotations(module, visitAnnotation); + AnnotationSet::removeAnnotations( + module, std::bind(visitAnnotation, -1, std::placeholders::_1)); } // If an element changed liveness then propagate liveness through it. @@ -744,11 +736,11 @@ void IMDeadCodeElimPass::rewriteModuleSignature(FModuleOp module) { for (auto index : llvm::seq(0u, numOldPorts)) { auto argument = module.getArgument(index); - assert((!shouldPreserveDontTouch(argument) || isKnownAlive(argument)) && + assert((!hasDontTouch(argument) || isKnownAlive(argument)) && "If the port has don't touch, it should be known alive"); // If the port has dontTouch, skip. - if (shouldPreserveDontTouch(argument)) + if (hasDontTouch(argument)) continue; if (isKnownAlive(argument)) { diff --git a/test/Dialect/FIRRTL/Reduction/port-pruner.mlir b/test/Dialect/FIRRTL/Reduction/imdce-remove-ports.mlir similarity index 100% rename from test/Dialect/FIRRTL/Reduction/port-pruner.mlir rename to test/Dialect/FIRRTL/Reduction/imdce-remove-ports.mlir diff --git a/test/Dialect/FIRRTL/remove-unused-ports.mlir b/test/Dialect/FIRRTL/imdce-remove-ports.mlir similarity index 94% rename from test/Dialect/FIRRTL/remove-unused-ports.mlir rename to test/Dialect/FIRRTL/imdce-remove-ports.mlir index 5b338239a3d1..8f799f5f38c3 100644 --- a/test/Dialect/FIRRTL/remove-unused-ports.mlir +++ b/test/Dialect/FIRRTL/imdce-remove-ports.mlir @@ -1,5 +1,4 @@ // RUN: circt-opt -pass-pipeline='builtin.module(firrtl-imdeadcodeelim{remove-ports-only})' %s -split-input-file | FileCheck %s -// RUN: circt-opt -pass-pipeline='builtin.module(firrtl-imdeadcodeelim{remove-ports-only ignore-dont-touch})' %s -split-input-file | FileCheck %s --check-prefix=IGNORE firrtl.circuit "Top" { // CHECK-LABEL: firrtl.module @Top(in %a: !firrtl.uint<1>, in %b: !firrtl.uint<1>, out %c: !firrtl.uint<1>, // CHECK-SAME: out %d_unused: !firrtl.uint<1>, out %d_invalid: !firrtl.uint<1>, out %d_constant: !firrtl.uint<1>) @@ -206,25 +205,6 @@ firrtl.circuit "ProbeTypes" { // ----- -// Test ignoreDontTouch option - removes unused ports even with symbols/annotations - -// IGNORE-LABEL: firrtl.circuit "IgnoreDontTouch" -firrtl.circuit "IgnoreDontTouch" { - firrtl.module @IgnoreDontTouch() { - // IGNORE: firrtl.instance bar @Bar() - %bar_sym, %bar_anno = firrtl.instance bar @Bar(out sym: !firrtl.uint<1>, out anno: !firrtl.uint<1>) - } - - // IGNORE-LABEL: firrtl.module private @Bar() { - // IGNORE-NEXT: } - firrtl.module private @Bar(out %sym: !firrtl.uint<1> sym @sym, out %anno: !firrtl.uint<1>) attributes { - portAnnotations = [[], [{a = "a"}]] - } { - } -} - -// ----- - // Test case for issue #10504 - property types with instances should not crash // This tests that unused property ports can be removed without assertion failures firrtl.circuit "PropertyTypes" { From c594fd8cd82796e0e687300951ba0b81e1172316 Mon Sep 17 00:00:00 2001 From: Hideto Ueno Date: Tue, 2 Jun 2026 17:47:22 -0700 Subject: [PATCH 3/4] Format --- lib/Dialect/FIRRTL/FIRRTLReductions.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp index 3627fd36f3a7..3a1e157b2b33 100644 --- a/lib/Dialect/FIRRTL/FIRRTLReductions.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLReductions.cpp @@ -2665,8 +2665,8 @@ struct ExtmoduleConventionRemover : public OpReduction { struct IMDCEPortReduction : public PassReduction { IMDCEPortReduction(MLIRContext *context) - : PassReduction(context, - firrtl::createIMDeadCodeElim({/*removePortsOnly=*/true})) { + : PassReduction( + context, firrtl::createIMDeadCodeElim({/*removePortsOnly=*/true})) { } std::string getName() const override { return "firrtl-imdeadcodeelim-remove-ports"; From 398c99e8c3ab8c0734a4f33dd3b532089d6d4a4b Mon Sep 17 00:00:00 2001 From: Hideto Ueno Date: Fri, 5 Jun 2026 15:55:50 -0700 Subject: [PATCH 4/4] Update test/Dialect/FIRRTL/imdce-remove-ports.mlir Co-authored-by: Schuyler Eldridge --- test/Dialect/FIRRTL/imdce-remove-ports.mlir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Dialect/FIRRTL/imdce-remove-ports.mlir b/test/Dialect/FIRRTL/imdce-remove-ports.mlir index 8f799f5f38c3..34dc9c8bca53 100644 --- a/test/Dialect/FIRRTL/imdce-remove-ports.mlir +++ b/test/Dialect/FIRRTL/imdce-remove-ports.mlir @@ -205,7 +205,7 @@ firrtl.circuit "ProbeTypes" { // ----- -// Test case for issue #10504 - property types with instances should not crash +// Test case for issue #10504. Property types with instances should not crash // This tests that unused property ports can be removed without assertion failures firrtl.circuit "PropertyTypes" { // CHECK-LABEL: firrtl.module @PropertyTypes