[HW/SV] Add hw.inout elimination pass (#5390)

Adds a pass which analyses the module hierarchy to eliminate `inout` ports in favor of:
1. poking holes through the module hierarchy (explicit in/output ports)
2. raising `sv.assign`/`sv.read_inout` operations to the definition point of the `hw.inout` value.

This is unarbited, and only allows for multiple-readers, single-writer scenarios. Currently only supports `sv.assign`, as well as `hw.inout` input ports.

include/circt/Dialect/HW/PortConverter.h

@@ -105,6 +105,11 @@ class PortConversion {
       : converter(converter), body(converter.getBody()), origPort(origPort) {}
   virtual ~PortConversion() = default;
 
+  // An optional initialization step that can be overridden by subclasses.
+  // This allows subclasses to perform a failable post-construction
+  // initialization step.
+  virtual LogicalResult init() { return success(); }
+
   // Lower the specified port into a wire-level signaling protocol. The two
   // virtual methods 'build*Signals' should be overridden by subclasses. They
   // should use the 'create*' methods in 'PortConverter' to create the
@@ -144,7 +149,7 @@ class PortConversion {
   // classes), we only need to dynamically tell whether any given PortConversion
   // is the UntouchedPortConversion.
   bool isUntouchedFlag = false;
-};
+}; // namespace hw
 
 // A PortConversionBuilder will, given an input type, build the appropriate
 // port conversion for that type.

include/circt/Dialect/SV/SVPasses.h

@@ -25,6 +25,7 @@ std::unique_ptr<mlir::Pass> createHWStubExternalModulesPass();
 std::unique_ptr<mlir::Pass> createHWLegalizeModulesPass();
 std::unique_ptr<mlir::Pass> createSVTraceIVerilogPass();
 std::unique_ptr<mlir::Pass> createHWGeneratorCalloutPass();
+std::unique_ptr<mlir::Pass> createHWEliminateInOutPortsPass();
 std::unique_ptr<mlir::Pass> createHWMemSimImplPass(
     bool replSeqMem = false, bool ignoreReadEnable = false,
     bool addMuxPragmas = false, bool disableMemRandomization = false,

include/circt/Dialect/SV/SVPasses.td

@@ -173,4 +173,17 @@ def HWExportModuleHierarchy : Pass<"hw-export-module-hierarchy",
   let dependentDialects = ["circt::sv::SVDialect"];
 }
 
+def HWEliminateInOutPorts : Pass<"hw-eliminate-inout-ports",
+                             "mlir::ModuleOp"> {
+  let summary = "Raises usages of inout ports into explicit input and output ports";
+  let description = [{
+    This pass raises usages of inout ports into explicit in- and output ports.
+    This is done by analyzing the usage of the inout ports and creating new
+    input and output ports at the using module, and subsequently moving the
+    inout read- and writes to the instantiation site.
+  }];
+  let constructor = "circt::sv::createHWEliminateInOutPortsPass()";
+  let dependentDialects = ["circt::sv::SVDialect"];
+}
+
 #endif // CIRCT_DIALECT_SV_SVPASSES

lib/Dialect/HW/PortConverter.cpp

@@ -42,7 +42,7 @@ class UntouchedPortConversion : public PortConversion {
   void mapOutputSignals(OpBuilder &b, Operation *inst, Value instValue,
                         SmallVectorImpl<Value> &newOperands,
                         ArrayRef<Backedge> newResults) override {
-    instValue.replaceAllUsesWith(newOperands[portInfo.argNum]);
+    instValue.replaceAllUsesWith(newResults[portInfo.argNum]);
   }
 
 private:
@@ -119,6 +119,10 @@ LogicalResult PortConverterImpl::run() {
 
     foundLoweredPorts |= !(*loweredPort)->isUntouched();
     loweredPorts.emplace_back(std::move(*loweredPort));
+
+    if (failed(loweredPorts.back()->init()))
+      return failure();
+
     return success();
   };
 

lib/Dialect/SV/Transforms/CMakeLists.txt

@@ -8,6 +8,7 @@ add_circt_dialect_library(CIRCTSVTransforms
   SVExtractTestCode.cpp
   HWExportModuleHierarchy.cpp
   SVTraceIVerilog.cpp
+  HWEliminateInOutPorts.cpp
 
   DEPENDS
   CIRCTSVTransformsIncGen

lib/Dialect/SV/Transforms/HWEliminateInOutPorts.cpp

@@ -0,0 +1,205 @@
+//===- HWEliminateInOutPorts.cpp - Generator Callout Pass
+//---------------------===//
+//
+// 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 "PassDetail.h"
+#include "circt/Dialect/HW/HWInstanceGraph.h"
+#include "circt/Dialect/HW/HWOps.h"
+#include "circt/Dialect/HW/PortConverter.h"
+#include "circt/Dialect/SV/SVPasses.h"
+#include "mlir/IR/Builders.h"
+#include "llvm/ADT/PostOrderIterator.h"
+
+using namespace circt;
+using namespace sv;
+using namespace hw;
+
+namespace {
+
+struct HWEliminateInOutPortsPass
+    : public sv::HWEliminateInOutPortsBase<HWEliminateInOutPortsPass> {
+  void runOnOperation() override;
+};
+} // end anonymous namespace
+
+namespace {
+
+class HWInOutPortConversion : public PortConversion {
+public:
+  HWInOutPortConversion(PortConverterImpl &converter, hw::PortInfo port);
+
+  void mapInputSignals(OpBuilder &b, Operation *inst, Value instValue,
+                       SmallVectorImpl<Value> &newOperands,
+                       ArrayRef<Backedge> newResults) override;
+  void mapOutputSignals(OpBuilder &b, Operation *inst, Value instValue,
+                        SmallVectorImpl<Value> &newOperands,
+                        ArrayRef<Backedge> newResults) override;
+
+  LogicalResult init() override;
+
+private:
+  void buildInputSignals() override;
+  void buildOutputSignals() override;
+
+  // Readers of this port internal in the module.
+  llvm::SmallVector<sv::ReadInOutOp, 4> readers;
+  // Writers of this port internal in the module.
+  llvm::SmallVector<sv::AssignOp, 4> writers;
+
+  bool hasReaders() { return !readers.empty(); }
+  bool hasWriters() { return !writers.empty(); }
+
+  // Handles to port info of the newly created ports.
+  PortInfo readPort, writePort;
+};
+
+HWInOutPortConversion::HWInOutPortConversion(PortConverterImpl &converter,
+                                             hw::PortInfo port)
+    : PortConversion(converter, port) {}
+
+LogicalResult HWInOutPortConversion::init() {
+  // Gather readers and writers (how to handle sv.passign?)
+  for (auto *user : body->getArgument(origPort.argNum).getUsers()) {
+    if (auto read = dyn_cast<sv::ReadInOutOp>(user))
+      readers.push_back(read);
+    else if (auto write = dyn_cast<sv::AssignOp>(user))
+      writers.push_back(write);
+    else
+      return user->emitOpError() << "uses hw.inout port " << origPort.name
+                                 << " but the operation itself is unsupported.";
+  }
+
+  if (writers.size() > 1)
+    return converter.getModule()->emitOpError()
+           << "multiple writers of inout port " << origPort.name
+           << " is unsupported.";
+
+  return success();
+}
+
+void HWInOutPortConversion::buildInputSignals() {
+  if (hasReaders()) {
+    // Replace all sv::ReadInOutOp's with the new input.
+    Value readValue =
+        converter.createNewInput(origPort, "_rd", origPort.type, readPort);
+    Value origInput = body->getArgument(origPort.argNum);
+    for (auto *user : llvm::make_early_inc_range(origInput.getUsers())) {
+      sv::ReadInOutOp read = dyn_cast<sv::ReadInOutOp>(user);
+      if (!read)
+        continue;
+
+      read.replaceAllUsesWith(readValue);
+      read.erase();
+    }
+  }
+
+  if (hasWriters()) {
+    // Replace the sv::AssignOp with the new output.
+    sv::AssignOp write = writers.front();
+    converter.createNewOutput(origPort, "_wr", origPort.type, write.getSrc(),
+                              writePort);
+    write.erase();
+  }
+}
+
+void HWInOutPortConversion::buildOutputSignals() {
+  assert(false && "`hw.inout` outputs not yet supported. Currently, `hw.inout` "
+                  "outputs are handled by UntouchedPortConversion, given that "
+                  "output `hw.inout` ports have a `PortDirection::Output` "
+                  "direction instead of `PortDirection::InOut`. If this for "
+                  "some reason changes, then this assert will fire.");
+}
+
+void HWInOutPortConversion::mapInputSignals(OpBuilder &b, Operation *inst,
+                                            Value instValue,
+                                            SmallVectorImpl<Value> &newOperands,
+                                            ArrayRef<Backedge> newResults) {
+
+  if (hasReaders()) {
+    // Create a read_inout op at the instantiation point. This effectively
+    // pushes the read_inout op from the module to the instantiation site.
+    newOperands[readPort.argNum] =
+        b.create<ReadInOutOp>(inst->getLoc(), instValue).getResult();
+  }
+
+  if (hasWriters()) {
+    // Create a sv::AssignOp at the instantiation point. This effectively
+    // pushes the write op from the module to the instantiation site.
+    Value writeFromInsideMod = newResults[writePort.argNum];
+    b.create<sv::AssignOp>(inst->getLoc(), instValue, writeFromInsideMod);
+  }
+}
+
+void HWInOutPortConversion::mapOutputSignals(
+    OpBuilder &b, Operation *inst, Value instValue,
+    SmallVectorImpl<Value> &newOperands, ArrayRef<Backedge> newResults) {
+  assert(false && "`hw.inout` outputs not yet supported. Currently, `hw.inout` "
+                  "outputs are handled by UntouchedPortConversion, given that "
+                  "output `hw.inout` ports have a `PortDirection::Output` "
+                  "direction instead of `PortDirection::InOut`. If this for "
+                  "some reason changes, then this assert will fire.");
+}
+
+class HWInoutPortConversionBuilder : public PortConversionBuilder {
+public:
+  using PortConversionBuilder::PortConversionBuilder;
+  FailureOr<std::unique_ptr<PortConversion>> build(hw::PortInfo port) override {
+    if (port.direction == hw::PortDirection::INOUT)
+      return {std::make_unique<HWInOutPortConversion>(converter, port)};
+    return PortConversionBuilder::build(port);
+  }
+};
+
+} // namespace
+
+void HWEliminateInOutPortsPass::runOnOperation() {
+  // Find all modules and run port conversion on them.
+  circt::hw::InstanceGraph &instanceGraph =
+      getAnalysis<circt::hw::InstanceGraph>();
+  llvm::DenseSet<InstanceGraphNode *> visited;
+  FailureOr<llvm::ArrayRef<InstanceGraphNode *>> res =
+      instanceGraph.getInferredTopLevelNodes();
+
+  if (failed(res)) {
+    signalPassFailure();
+    return;
+  }
+
+  // Visit the instance hierarchy in a depth-first manner, modifying child
+  // modules and their ports before their parents.
+
+  // Doing this DFS ensures that all module instance uses of an inout value has
+  // been converted before the current instance use. E.g. say you have m1 -> m2
+  // -> m3 where both m3 and m2 reads an inout value defined in m1. If we don't
+  // do DFS, and we just randomly pick a module, we have to e.g. select m2, see
+  // that it also passes that inout value to other module instances, processes
+  // those first (which may bubble up read/writes to that hw.inout op), and then
+  // process m2... which in essence is a DFS traversal. So we just go ahead and
+  // do the DFS to begin with, ensuring the invariant that all module instance
+  // uses of an inout value have been converted before converting any given
+  // module.
+
+  for (InstanceGraphNode *topModule : res.value()) {
+    for (InstanceGraphNode *node : llvm::post_order(topModule)) {
+      if (visited.count(node))
+        continue;
+      auto mutableModule =
+          dyn_cast_or_null<hw::HWMutableModuleLike>(*node->getModule());
+      if (!mutableModule)
+        continue;
+      if (failed(PortConverter<HWInoutPortConversionBuilder>(instanceGraph,
+                                                             mutableModule)
+                     .run()))
+        return signalPassFailure();
+    }
+  }
+}
+
+std::unique_ptr<Pass> circt::sv::createHWEliminateInOutPortsPass() {
+  return std::make_unique<HWEliminateInOutPortsPass>();
+}

test/Dialect/SV/hw-eliminate-inout-ports-errors.mlir

@@ -0,0 +1,15 @@
+// RUN: circt-opt --split-input-file -allow-unregistered-dialect --hw-eliminate-inout-ports -verify-diagnostics %s
+
+hw.module @unsupported(%a: !hw.inout<i42>) {
+  // expected-error @+1 {{uses hw.inout port "a" but the operation itself is unsupported.}}
+  "foo.bar" (%a) : (!hw.inout<i42>) -> ()
+}
+
+// -----
+
+// expected-error @+1 {{multiple writers of inout port "a" is unsupported.}}
+hw.module @multipleWriters(%a: !hw.inout<i42>) {
+  %0 = hw.constant 0 : i42
+  sv.assign %a, %0 : i42
+  sv.assign %a, %0 : i42
+}

test/Dialect/SV/hw-eliminate-inout-ports.mlir

@@ -0,0 +1,95 @@
+// RUN: circt-opt --hw-eliminate-inout-ports %s | FileCheck %s
+
+// CHECK-LABEL:   hw.module @read(
+// CHECK-SAME:                    %[[VAL_0:.*]]: i42) -> (out: i42) {
+// CHECK:           hw.output %[[VAL_0]] : i42
+// CHECK:         }
+hw.module @read(%a: !hw.inout<i42>) -> (out: i42) {
+  %aget = sv.read_inout %a: !hw.inout<i42>
+  hw.output %aget : i42
+}
+
+// CHECK-LABEL:   hw.module @write() -> (a_wr: i42) {
+// CHECK:           %[[VAL_0:.*]] = hw.constant 0 : i42
+// CHECK:           hw.output %[[VAL_0]] : i42
+// CHECK:         }
+hw.module @write(%a: !hw.inout<i42>) {
+  %0 = hw.constant 0 : i42
+  sv.assign %a, %0 : i42
+}
+
+// CHECK-LABEL:   hw.module @read_write(
+// CHECK-SAME:                          %[[VAL_0:.*]]: i42) -> (a_wr: i42, out: i42) {
+// CHECK:           hw.output %[[VAL_0]], %[[VAL_0]] : i42, i42
+// CHECK:         }
+hw.module @read_write(%a: !hw.inout<i42>) -> (out: i42) {
+  %aget = sv.read_inout %a: !hw.inout<i42>
+  sv.assign %a, %aget : i42
+  hw.output %aget : i42
+}
+
+// CHECK-LABEL:   hw.module @oneLevel() {
+// CHECK:           %[[VAL_0:.*]] = sv.wire : !hw.inout<i42>
+// CHECK:           %[[VAL_1:.*]] = sv.read_inout %[[VAL_0]] : !hw.inout<i42>
+// CHECK:           %[[VAL_2:.*]] = hw.instance "read" @read(a_rd: %[[VAL_1]]: i42) -> (out: i42)
+// CHECK:           sv.assign %[[VAL_0]], %[[VAL_3:.*]] : i42
+// CHECK:           %[[VAL_3]] = hw.instance "write" @write() -> (a_wr: i42)
+// CHECK:           %[[VAL_4:.*]] = sv.read_inout %[[VAL_0]] : !hw.inout<i42>
+// CHECK:           sv.assign %[[VAL_0]], %[[VAL_5:.*]] : i42
+// CHECK:           %[[VAL_5]], %[[VAL_6:.*]] = hw.instance "readWrite" @read_write(a_rd: %[[VAL_4]]: i42) -> (a_wr: i42, out: i42)
+// CHECK:           hw.output
+// CHECK:         }
+hw.module @oneLevel() {
+  // No error here, even though the inout is written in two places. The
+  // pass will only error upon the recursive case when it inspects a module
+  // and sees that there are multiple writers to an inout *port*.
+  %0 = sv.wire : !hw.inout<i42>
+  %read = hw.instance "read" @read(a : %0 : !hw.inout<i42>) -> (out: i42)
+  hw.instance "write" @write(a : %0 : !hw.inout<i42>) -> ()
+  %read_write = hw.instance "readWrite" @read_write(a : %0 : !hw.inout<i42>) -> (out: i42)
+}
+
+
+// CHECK-LABEL:   hw.module @passthrough() -> (a_wr: i42) {
+// CHECK:           %[[VAL_0:.*]] = hw.instance "write" @write() -> (a_wr: i42)
+// CHECK:           hw.output %[[VAL_0]] : i42
+// CHECK:         }
+hw.module @passthrough(%a : !hw.inout<i42>) -> () {
+  hw.instance "write" @write(a : %a : !hw.inout<i42>) -> ()
+}
+
+// CHECK-LABEL:   hw.module @passthroughTwoLevels() {
+// CHECK:           %[[VAL_0:.*]] = sv.wire : !hw.inout<i42>
+// CHECK:           sv.assign %[[VAL_0]], %[[VAL_1:.*]] : i42
+// CHECK:           %[[VAL_1]] = hw.instance "passthrough" @passthrough() -> (a_wr: i42)
+// CHECK:           hw.output
+// CHECK:         }
+hw.module @passthroughTwoLevels() {
+  %0 = sv.wire : !hw.inout<i42>
+  hw.instance "passthrough" @passthrough(a : %0 : !hw.inout<i42>) -> ()
+}
+
+
+// For now, we don't support/touch inout ports. We add a test here to add
+// an early detection signal if something changes in the inout port handling
+// logic in CIRCT.
+
+// CHECK-LABEL:   hw.module @outputInout() -> (out: !hw.inout<i42>) {
+// CHECK:           %[[VAL_0:.*]] = sv.wire : !hw.inout<i42>
+// CHECK:           hw.output %[[VAL_0]] : !hw.inout<i42>
+// CHECK:         }
+hw.module @outputInout() -> (out : !hw.inout<i42>) {
+  %0 = sv.wire : !hw.inout<i42>
+  hw.output %0 : !hw.inout<i42>
+}
+
+// CHECK-LABEL:   hw.module @outputInoutDriver() {
+// CHECK:           %[[VAL_0:.*]] = hw.instance "outputInout" @outputInout() -> (out: !hw.inout<i42>)
+// CHECK:           sv.assign %[[VAL_0]], %[[VAL_1:.*]] : i42
+// CHECK:           %[[VAL_1]] = hw.instance "write" @write() -> (a_wr: i42)
+// CHECK:           hw.output
+// CHECK:         }
+hw.module @outputInoutDriver() {
+  %0 = hw.instance "outputInout" @outputInout() -> (out : !hw.inout<i42>)
+  hw.instance "write" @write(a : %0 : !hw.inout<i42>) -> ()
+}