From 71011a70a4973e94d507bc5d4f10f46c25d29793 Mon Sep 17 00:00:00 2001 From: John Demme Date: Tue, 24 Jun 2025 18:09:44 -0700 Subject: [PATCH] [Kanagawa] Replace CSE with specialized pass (#8599) Running CSE on a whole design is neither necessary nor advisable. It forces all the dialects used to support CSE but at least one (which we use) has issues with it. So we add a PR which targets redundant operations in the `kanagawa` dialect which passes assume to be non-redundant. Also updates `kanagawatool` to add new passes which are now necessary. --- .../circt/Dialect/Kanagawa/KanagawaPasses.h | 1 + .../circt/Dialect/Kanagawa/KanagawaPasses.td | 14 +++ .../Kanagawa/Transforms/CMakeLists.txt | 1 + .../Transforms/KanagawaCleanSelfdrivers.cpp | 10 +- .../KanagawaEliminateRedundantOps.cpp | 118 ++++++++++++++++++ .../Transforms/KanagawaPassPipelines.cpp | 12 +- .../Transforms/eliminate_redundant_ops.mlir | 60 +++++++++ tools/kanagawatool/CMakeLists.txt | 2 + tools/kanagawatool/kanagawatool.cpp | 5 + 9 files changed, 215 insertions(+), 8 deletions(-) create mode 100644 lib/Dialect/Kanagawa/Transforms/KanagawaEliminateRedundantOps.cpp create mode 100644 test/Dialect/Kanagawa/Transforms/eliminate_redundant_ops.mlir diff --git a/include/circt/Dialect/Kanagawa/KanagawaPasses.h b/include/circt/Dialect/Kanagawa/KanagawaPasses.h index fe0271bbf6..d917a6f007 100644 --- a/include/circt/Dialect/Kanagawa/KanagawaPasses.h +++ b/include/circt/Dialect/Kanagawa/KanagawaPasses.h @@ -32,6 +32,7 @@ createTunnelingPass(const KanagawaTunnelingOptions & = {}); std::unique_ptr createPortrefLoweringPass(); std::unique_ptr createCleanSelfdriversPass(); std::unique_ptr createContainersToHWPass(); +std::unique_ptr createEliminateRedundantOpsPass(); std::unique_ptr createArgifyBlocksPass(); std::unique_ptr createReblockPass(); std::unique_ptr createInlineSBlocksPass(); diff --git a/include/circt/Dialect/Kanagawa/KanagawaPasses.td b/include/circt/Dialect/Kanagawa/KanagawaPasses.td index bbbd247315..91927a4bac 100644 --- a/include/circt/Dialect/Kanagawa/KanagawaPasses.td +++ b/include/circt/Dialect/Kanagawa/KanagawaPasses.td @@ -103,6 +103,20 @@ def KanagawaContainersToHW : Pass<"kanagawa-convert-containers-to-hw", "::mlir:: let dependentDialects = ["::circt::hw::HWDialect"]; } +def KanagawaEliminateRedundantOps : Pass<"kanagawa-eliminate-redundant-ops", "kanagawa::ContainerOp"> { + let summary = "Kanagawa eliminate redundant operations pass"; + let description = [{ + Eliminates redundant operations within Kanagawa containers to optimize the IR. + This pass analyzes operations within containers and removes unnecessary or + duplicate operations that do not affect the semantic behavior. + + Redundant operations can (read: will) cause issues in other passes. So this + pass needs to be run after any pass which can introduce redundant + operations. + }]; + let constructor = "::circt::kanagawa::createEliminateRedundantOpsPass()"; +} + def KanagawaArgifyBlocks : Pass<"kanagawa-argify-blocks"> { let summary = "Add arguments to kanagawa blocks"; let description = [{ diff --git a/lib/Dialect/Kanagawa/Transforms/CMakeLists.txt b/lib/Dialect/Kanagawa/Transforms/CMakeLists.txt index 5d4e25de4f..318588e989 100644 --- a/lib/Dialect/Kanagawa/Transforms/CMakeLists.txt +++ b/lib/Dialect/Kanagawa/Transforms/CMakeLists.txt @@ -14,6 +14,7 @@ add_circt_dialect_library(CIRCTKanagawaTransforms KanagawaConvertHandshakeToDC.cpp KanagawaMethodsToContainers.cpp KanagawaAddOperatorLibrary.cpp + KanagawaEliminateRedundantOps.cpp DEPENDS CIRCTKanagawaTransformsIncGen diff --git a/lib/Dialect/Kanagawa/Transforms/KanagawaCleanSelfdrivers.cpp b/lib/Dialect/Kanagawa/Transforms/KanagawaCleanSelfdrivers.cpp index 1bc0aa1c1f..fbb91faf0e 100644 --- a/lib/Dialect/Kanagawa/Transforms/KanagawaCleanSelfdrivers.cpp +++ b/lib/Dialect/Kanagawa/Transforms/KanagawaCleanSelfdrivers.cpp @@ -65,8 +65,9 @@ static LogicalResult replaceReadsOfWrites(ContainerOp containerOp) { [getPortOp.getPortSymbolAttr().getAttr()]; if (getPortOp.getDirection() == Direction::Input) { if (portAccesses.getAsInput) - return containerOp.emitError( - "multiple input get_ports - please CSE the input IR"); + return portAccesses.getAsInput.emitError("multiple input get_ports") + .attachNote(getPortOp.getLoc()) + << "redundant get_port here"; portAccesses.getAsInput = getPortOp; for (auto *user : getPortOp->getUsers()) { if (auto writer = dyn_cast(user)) { @@ -78,8 +79,9 @@ static LogicalResult replaceReadsOfWrites(ContainerOp containerOp) { } } else { if (portAccesses.getAsOutput) - return containerOp.emitError( - "multiple get_port as output - please CSE the input IR"); + return portAccesses.getAsOutput.emitError("multiple get_port as output") + .attachNote(getPortOp.getLoc()) + << "redundant get_port here"; portAccesses.getAsOutput = getPortOp; for (auto *user : getPortOp->getUsers()) { diff --git a/lib/Dialect/Kanagawa/Transforms/KanagawaEliminateRedundantOps.cpp b/lib/Dialect/Kanagawa/Transforms/KanagawaEliminateRedundantOps.cpp new file mode 100644 index 0000000000..e54f28b8c4 --- /dev/null +++ b/lib/Dialect/Kanagawa/Transforms/KanagawaEliminateRedundantOps.cpp @@ -0,0 +1,118 @@ +//===- KanagawaEliminateRedundantOps.cpp ----------------------------------===// +// +// 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/Kanagawa/KanagawaDialect.h" +#include "circt/Dialect/Kanagawa/KanagawaOps.h" +#include "circt/Dialect/Kanagawa/KanagawaPasses.h" +#include "circt/Dialect/Kanagawa/KanagawaTypes.h" + +#include "mlir/Pass/Pass.h" + +#include "llvm/ADT/TypeSwitch.h" +#include "llvm/Support/Debug.h" + +#define DEBUG_TYPE "kanagawa-eliminate-redundant-ops" + +namespace circt { +namespace kanagawa { +#define GEN_PASS_DEF_KANAGAWAELIMINATEREDUNDANTOPS +#include "circt/Dialect/Kanagawa/KanagawaPasses.h.inc" +} // namespace kanagawa +} // namespace circt + +using namespace circt; +using namespace kanagawa; + +// Helper function to eliminate redundant GetPortOps in a container +static void eliminateRedundantGetPortOps(ContainerOp containerOp) { + // Structure to track accesses to each port of each instance + struct PortAccesses { + GetPortOp getAsInput; + GetPortOp getAsOutput; + }; + + llvm::DenseMap> + instancePortAccessMap; + + // Collect all GetPortOps and identify redundant ones + llvm::SmallVector redundantOps; + + for (auto getPortOp : containerOp.getOps()) { + PortAccesses &portAccesses = + instancePortAccessMap[getPortOp.getInstance()] + [getPortOp.getPortSymbolAttr().getAttr()]; + + if (getPortOp.getDirection() == Direction::Input) { + if (portAccesses.getAsInput) { + // Found redundant input GetPortOp - mark the current one for removal + // and replace its uses with the existing one + getPortOp.replaceAllUsesWith(portAccesses.getAsInput.getResult()); + redundantOps.push_back(getPortOp); + } else { + portAccesses.getAsInput = getPortOp; + } + } else { + if (portAccesses.getAsOutput) { + // Found redundant output GetPortOp - mark the current one for removal + // and replace its uses with the existing one + getPortOp.replaceAllUsesWith(portAccesses.getAsOutput.getResult()); + redundantOps.push_back(getPortOp); + } else { + portAccesses.getAsOutput = getPortOp; + } + } + } + + // Remove all redundant GetPortOps + for (auto redundantOp : redundantOps) + redundantOp.erase(); +} + +// Helper function to eliminate redundant PortReadOps in a container +static void eliminateRedundantPortReadOps(ContainerOp containerOp) { + // Map to track the first PortReadOp for each port being read from + llvm::DenseMap portFirstReadMap; + llvm::SmallVector redundantOps; + + for (auto portReadOp : containerOp.getOps()) { + Value portBeingRead = + portReadOp.getPort(); // The port operand being read from + + if (auto existingRead = portFirstReadMap.lookup(portBeingRead)) { + // Found redundant PortReadOp - mark the current one for removal + // and replace its uses with the existing one + portReadOp.replaceAllUsesWith(existingRead.getResult()); + redundantOps.push_back(portReadOp); + } else { + portFirstReadMap[portBeingRead] = portReadOp; + } + } + + // Remove all redundant PortReadOps + for (auto redundantOp : redundantOps) + redundantOp.erase(); +} + +namespace { + +struct EliminateRedundantOpsPass + : public circt::kanagawa::impl::KanagawaEliminateRedundantOpsBase< + EliminateRedundantOpsPass> { + void runOnOperation() override { + ContainerOp containerOp = getOperation(); + eliminateRedundantGetPortOps(containerOp); + eliminateRedundantPortReadOps(containerOp); + } +}; + +} // anonymous namespace + +std::unique_ptr circt::kanagawa::createEliminateRedundantOpsPass() { + return std::make_unique(); +} diff --git a/lib/Dialect/Kanagawa/Transforms/KanagawaPassPipelines.cpp b/lib/Dialect/Kanagawa/Transforms/KanagawaPassPipelines.cpp index d953e5a53a..5f73641be5 100644 --- a/lib/Dialect/Kanagawa/Transforms/KanagawaPassPipelines.cpp +++ b/lib/Dialect/Kanagawa/Transforms/KanagawaPassPipelines.cpp @@ -34,15 +34,19 @@ void circt::kanagawa::loadKanagawaLowLevelPassPipeline(mlir::PassManager &pm) { pm.nest().addPass(createContainerizePass()); pm.addPass(hw::createVerifyInnerRefNamespacePass()); - // Pre-tunneling CSE pass. This ensures that duplicate get_port calls are - // removed before we start tunneling - no reason to tunnel the same thing - // twice. - pm.addPass(mlir::createCSEPass()); + // This pass ensures that duplicate get_port calls are removed before we + // start tunneling - no reason to tunnel the same thing twice. + pm.nest().nest().addPass( + createEliminateRedundantOpsPass()); + pm.nest().addPass( createTunnelingPass(KanagawaTunnelingOptions{"", ""})); pm.addPass(hw::createVerifyInnerRefNamespacePass()); pm.addPass(createPortrefLoweringPass()); pm.addPass(createSimpleCanonicalizerPass()); + // Run this again as some of the above passes may create redundant ops. + pm.nest().nest().addPass( + createEliminateRedundantOpsPass()); pm.nest().addPass(createCleanSelfdriversPass()); pm.addPass(createContainersToHWPass()); pm.addPass(hw::createVerifyInnerRefNamespacePass()); diff --git a/test/Dialect/Kanagawa/Transforms/eliminate_redundant_ops.mlir b/test/Dialect/Kanagawa/Transforms/eliminate_redundant_ops.mlir new file mode 100644 index 0000000000..cf280204db --- /dev/null +++ b/test/Dialect/Kanagawa/Transforms/eliminate_redundant_ops.mlir @@ -0,0 +1,60 @@ +// RUN: circt-opt --split-input-file --pass-pipeline="builtin.module(kanagawa.design(kanagawa.container(kanagawa-eliminate-redundant-ops)))" %s | FileCheck %s + +kanagawa.design @TestDesign { + +// Test case 1: Eliminate redundant GetPortOps for input ports +// CHECK-LABEL: kanagawa.container sym @RedundantInputGetPort { +// CHECK-COUNT-1: kanagawa.get_port %{{.*}}, @in +// CHECK-NOT: kanagawa.get_port %{{.*}}, @in +kanagawa.container sym @RedundantInputGetPort { + %instance1 = kanagawa.container.instance @child, <@TestDesign::@ChildContainer> + + // First GetPortOp for input port - should be kept + %port_ref1 = kanagawa.get_port %instance1, @in : !kanagawa.scoperef<@TestDesign::@ChildContainer> -> !kanagawa.portref + + // Second GetPortOp for same input port - should be eliminated + %port_ref2 = kanagawa.get_port %instance1, @in : !kanagawa.scoperef<@TestDesign::@ChildContainer> -> !kanagawa.portref + + // Third GetPortOp for same input port - should be eliminated + %port_ref3 = kanagawa.get_port %instance1, @in : !kanagawa.scoperef<@TestDesign::@ChildContainer> -> !kanagawa.portref +} + +kanagawa.container sym @ChildContainer { + %in = kanagawa.port.input "in" sym @in : i32 +} + +} + +// ----- + +kanagawa.design @TestDesign2 { + +// Test case 2: Eliminate redundant GetPortOps for output ports and port reads. +// CHECK-LABEL: kanagawa.container sym @RedundantOutputGetPort { +// CHECK-COUNT-1: kanagawa.get_port %{{.*}}, @out +// CHECK-NOT: kanagawa.get_port %{{.*}}, @out +// CHECK-COUNT-1: kanagawa.port.read %{{.*}} : !kanagawa.portref +// CHECK-NOT: kanagawa.port.read %{{.*}} : !kanagawa.portref +kanagawa.container sym @RedundantOutputGetPort { + %instance1 = kanagawa.container.instance @child, <@TestDesign2::@ChildContainer> + + // First GetPortOp for output port - should be kept + %port_ref1 = kanagawa.get_port %instance1, @out : !kanagawa.scoperef<@TestDesign2::@ChildContainer> -> !kanagawa.portref + + // Second GetPortOp for same output port - should be eliminated + %port_ref2 = kanagawa.get_port %instance1, @out : !kanagawa.scoperef<@TestDesign2::@ChildContainer> -> !kanagawa.portref + + %val1 = kanagawa.port.read %port_ref1 : !kanagawa.portref + %val2 = kanagawa.port.read %port_ref2 : !kanagawa.portref + + %sum = arith.addi %val1, %val2 : i32 +} + +kanagawa.container sym @ChildContainer { + %in = kanagawa.port.input "in" sym @in : i32 + %out = kanagawa.port.output "out" sym @out : i32 + %const = hw.constant 100 : i32 + kanagawa.port.write %out, %const : !kanagawa.portref +} + +} diff --git a/tools/kanagawatool/CMakeLists.txt b/tools/kanagawatool/CMakeLists.txt index e149f07cb2..987b694cc1 100644 --- a/tools/kanagawatool/CMakeLists.txt +++ b/tools/kanagawatool/CMakeLists.txt @@ -24,6 +24,8 @@ target_link_libraries(kanagawatool CIRCTSVTransforms CIRCTKanagawa CIRCTKanagawaTransforms + CIRCTCombTransforms + CIRCTHWToSV CIRCTTransforms CIRCTPipelineOps CIRCTPipelineToHW diff --git a/tools/kanagawatool/kanagawatool.cpp b/tools/kanagawatool/kanagawatool.cpp index a4e3f6e8dc..467aab6f80 100644 --- a/tools/kanagawatool/kanagawatool.cpp +++ b/tools/kanagawatool/kanagawatool.cpp @@ -39,7 +39,9 @@ #include "llvm/Support/ToolOutputFile.h" #include "circt/Conversion/ExportVerilog.h" +#include "circt/Conversion/HWToSV.h" #include "circt/Conversion/Passes.h" +#include "circt/Dialect/Comb/CombPasses.h" #include "circt/Dialect/DC/DCDialect.h" #include "circt/Dialect/DC/DCPasses.h" #include "circt/Dialect/ESI/ESIDialect.h" @@ -184,6 +186,7 @@ static void loadDCTransformsPipeline(OpPassManager &pm) { } static void loadESILoweringPipeline(OpPassManager &pm) { + pm.addPass(circt::esi::createESIBundleLoweringPass()); pm.addPass(circt::esi::createESIPortLoweringPass()); pm.addPass(circt::esi::createESIPhysicalLoweringPass()); pm.addPass(circt::esi::createESItoHWPass()); @@ -196,6 +199,8 @@ static void loadHWLoweringPipeline(OpPassManager &pm) { pm.addPass(circt::createLowerSeqToSVPass()); pm.nest().addPass(sv::createHWCleanupPass()); pm.addPass(mlir::createCSEPass()); + pm.addPass(circt::comb::createLowerComb()); + pm.nest().addPass(circt::createLowerHWToSVPass()); // Legalize unsupported operations within the modules. pm.nest().addPass(sv::createHWLegalizeModulesPass());