[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.

include/circt/Dialect/Kanagawa/KanagawaPasses.h

@@ -32,6 +32,7 @@ createTunnelingPass(const KanagawaTunnelingOptions & = {});
 std::unique_ptr<mlir::Pass> createPortrefLoweringPass();
 std::unique_ptr<mlir::Pass> createCleanSelfdriversPass();
 std::unique_ptr<mlir::Pass> createContainersToHWPass();
+std::unique_ptr<mlir::Pass> createEliminateRedundantOpsPass();
 std::unique_ptr<mlir::Pass> createArgifyBlocksPass();
 std::unique_ptr<mlir::Pass> createReblockPass();
 std::unique_ptr<mlir::Pass> createInlineSBlocksPass();

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 = [{

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

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(
+        return portAccesses.getAsInput.emitError("multiple input get_ports")
-            "multiple input get_ports - please CSE the input IR");
+                   .attachNote(getPortOp.getLoc())
+               << "redundant get_port here";
       portAccesses.getAsInput = getPortOp;
       for (auto *user : getPortOp->getUsers()) {
         if (auto writer = dyn_cast<PortWriteOp>(user)) {
@@ -78,8 +79,9 @@ static LogicalResult replaceReadsOfWrites(ContainerOp containerOp) {
       }
     } else {
       if (portAccesses.getAsOutput)
-        return containerOp.emitError(
+        return portAccesses.getAsOutput.emitError("multiple get_port as output")
-            "multiple get_port as output - please CSE the input IR");
+                   .attachNote(getPortOp.getLoc())
+               << "redundant get_port here";
       portAccesses.getAsOutput = getPortOp;
 
       for (auto *user : getPortOp->getUsers()) {

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</*instance*/ Value,
+                 /*portName*/ llvm::DenseMap<StringAttr, PortAccesses>>
+      instancePortAccessMap;
+
+  // Collect all GetPortOps and identify redundant ones
+  llvm::SmallVector<GetPortOp> redundantOps;
+
+  for (auto getPortOp : containerOp.getOps<GetPortOp>()) {
+    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<Value, PortReadOp> portFirstReadMap;
+  llvm::SmallVector<PortReadOp> redundantOps;
+
+  for (auto portReadOp : containerOp.getOps<PortReadOp>()) {
+    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<mlir::Pass> circt::kanagawa::createEliminateRedundantOpsPass() {
+  return std::make_unique<EliminateRedundantOpsPass>();
+}

lib/Dialect/Kanagawa/Transforms/KanagawaPassPipelines.cpp

@@ -34,15 +34,19 @@ void circt::kanagawa::loadKanagawaLowLevelPassPipeline(mlir::PassManager &pm) {
   pm.nest<kanagawa::DesignOp>().addPass(createContainerizePass());
   pm.addPass(hw::createVerifyInnerRefNamespacePass());
 
-  // Pre-tunneling CSE pass. This ensures that duplicate get_port calls are
+  // This pass ensures that duplicate get_port calls are removed before we
-  // removed before we start tunneling - no reason to tunnel the same thing
+  // start tunneling - no reason to tunnel the same thing twice.
-  // twice.
+  pm.nest<DesignOp>().nest<ContainerOp>().addPass(
-  pm.addPass(mlir::createCSEPass());
+      createEliminateRedundantOpsPass());
+
   pm.nest<DesignOp>().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<DesignOp>().nest<ContainerOp>().addPass(
+      createEliminateRedundantOpsPass());
   pm.nest<DesignOp>().addPass(createCleanSelfdriversPass());
   pm.addPass(createContainersToHWPass());
   pm.addPass(hw::createVerifyInnerRefNamespacePass());

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<in i32>
+
+  // Second GetPortOp for same input port - should be eliminated
+  %port_ref2 = kanagawa.get_port %instance1, @in : !kanagawa.scoperef<@TestDesign::@ChildContainer> -> !kanagawa.portref<in i32>
+
+  // Third GetPortOp for same input port - should be eliminated
+  %port_ref3 = kanagawa.get_port %instance1, @in : !kanagawa.scoperef<@TestDesign::@ChildContainer> -> !kanagawa.portref<in i32>
+}
+
+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<out i32>
+// CHECK-NOT: kanagawa.port.read %{{.*}} : !kanagawa.portref<out i32>
+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<out i32>
+
+  // Second GetPortOp for same output port - should be eliminated
+  %port_ref2 = kanagawa.get_port %instance1, @out : !kanagawa.scoperef<@TestDesign2::@ChildContainer> -> !kanagawa.portref<out i32>
+
+  %val1 = kanagawa.port.read %port_ref1 : !kanagawa.portref<out i32>
+  %val2 = kanagawa.port.read %port_ref2 : !kanagawa.portref<out i32>
+
+  %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<out i32>
+}
+
+}

tools/kanagawatool/CMakeLists.txt

@@ -24,6 +24,8 @@ target_link_libraries(kanagawatool
     CIRCTSVTransforms
     CIRCTKanagawa
     CIRCTKanagawaTransforms
+    CIRCTCombTransforms
+    CIRCTHWToSV
     CIRCTTransforms
     CIRCTPipelineOps
     CIRCTPipelineToHW

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<hw::HWModuleOp>().addPass(sv::createHWCleanupPass());
   pm.addPass(mlir::createCSEPass());
+  pm.addPass(circt::comb::createLowerComb());
+  pm.nest<hw::HWModuleOp>().addPass(circt::createLowerHWToSVPass());
 
   // Legalize unsupported operations within the modules.
   pm.nest<hw::HWModuleOp>().addPass(sv::createHWLegalizeModulesPass());