From 0e4774483400efe4f1199a4317e86c28e94b3a87 Mon Sep 17 00:00:00 2001 From: Morten Borup Petersen Date: Wed, 12 Jul 2023 10:44:29 +0200 Subject: [PATCH] [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 | 7 +- include/circt/Dialect/SV/SVPasses.h | 1 + include/circt/Dialect/SV/SVPasses.td | 13 ++ lib/Dialect/HW/PortConverter.cpp | 6 +- lib/Dialect/SV/Transforms/CMakeLists.txt | 1 + .../SV/Transforms/HWEliminateInOutPorts.cpp | 205 ++++++++++++++++++ .../SV/hw-eliminate-inout-ports-errors.mlir | 15 ++ test/Dialect/SV/hw-eliminate-inout-ports.mlir | 95 ++++++++ 8 files changed, 341 insertions(+), 2 deletions(-) create mode 100644 lib/Dialect/SV/Transforms/HWEliminateInOutPorts.cpp create mode 100644 test/Dialect/SV/hw-eliminate-inout-ports-errors.mlir create mode 100644 test/Dialect/SV/hw-eliminate-inout-ports.mlir diff --git a/include/circt/Dialect/HW/PortConverter.h b/include/circt/Dialect/HW/PortConverter.h index a94ff73907cd..6d454b2c4a5a 100644 --- a/include/circt/Dialect/HW/PortConverter.h +++ b/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. diff --git a/include/circt/Dialect/SV/SVPasses.h b/include/circt/Dialect/SV/SVPasses.h index 88d6f13061a3..343c6bda3001 100644 --- a/include/circt/Dialect/SV/SVPasses.h +++ b/include/circt/Dialect/SV/SVPasses.h @@ -25,6 +25,7 @@ std::unique_ptr createHWStubExternalModulesPass(); std::unique_ptr createHWLegalizeModulesPass(); std::unique_ptr createSVTraceIVerilogPass(); std::unique_ptr createHWGeneratorCalloutPass(); +std::unique_ptr createHWEliminateInOutPortsPass(); std::unique_ptr createHWMemSimImplPass( bool replSeqMem = false, bool ignoreReadEnable = false, bool addMuxPragmas = false, bool disableMemRandomization = false, diff --git a/include/circt/Dialect/SV/SVPasses.td b/include/circt/Dialect/SV/SVPasses.td index 4f36040260ea..507cc64f8046 100644 --- a/include/circt/Dialect/SV/SVPasses.td +++ b/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 diff --git a/lib/Dialect/HW/PortConverter.cpp b/lib/Dialect/HW/PortConverter.cpp index 32da3a682df4..d68b2f5a6297 100644 --- a/lib/Dialect/HW/PortConverter.cpp +++ b/lib/Dialect/HW/PortConverter.cpp @@ -42,7 +42,7 @@ class UntouchedPortConversion : public PortConversion { void mapOutputSignals(OpBuilder &b, Operation *inst, Value instValue, SmallVectorImpl &newOperands, ArrayRef 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(); }; diff --git a/lib/Dialect/SV/Transforms/CMakeLists.txt b/lib/Dialect/SV/Transforms/CMakeLists.txt index 76a9918c9771..c25962c24e70 100644 --- a/lib/Dialect/SV/Transforms/CMakeLists.txt +++ b/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 diff --git a/lib/Dialect/SV/Transforms/HWEliminateInOutPorts.cpp b/lib/Dialect/SV/Transforms/HWEliminateInOutPorts.cpp new file mode 100644 index 000000000000..a0a2f0fd6f0c --- /dev/null +++ b/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 { + 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 &newOperands, + ArrayRef newResults) override; + void mapOutputSignals(OpBuilder &b, Operation *inst, Value instValue, + SmallVectorImpl &newOperands, + ArrayRef newResults) override; + + LogicalResult init() override; + +private: + void buildInputSignals() override; + void buildOutputSignals() override; + + // Readers of this port internal in the module. + llvm::SmallVector readers; + // Writers of this port internal in the module. + llvm::SmallVector 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(user)) + readers.push_back(read); + else if (auto write = dyn_cast(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(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 &newOperands, + ArrayRef 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(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(inst->getLoc(), instValue, writeFromInsideMod); + } +} + +void HWInOutPortConversion::mapOutputSignals( + OpBuilder &b, Operation *inst, Value instValue, + SmallVectorImpl &newOperands, ArrayRef 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> build(hw::PortInfo port) override { + if (port.direction == hw::PortDirection::INOUT) + return {std::make_unique(converter, port)}; + return PortConversionBuilder::build(port); + } +}; + +} // namespace + +void HWEliminateInOutPortsPass::runOnOperation() { + // Find all modules and run port conversion on them. + circt::hw::InstanceGraph &instanceGraph = + getAnalysis(); + llvm::DenseSet visited; + FailureOr> 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(*node->getModule()); + if (!mutableModule) + continue; + if (failed(PortConverter(instanceGraph, + mutableModule) + .run())) + return signalPassFailure(); + } + } +} + +std::unique_ptr circt::sv::createHWEliminateInOutPortsPass() { + return std::make_unique(); +} diff --git a/test/Dialect/SV/hw-eliminate-inout-ports-errors.mlir b/test/Dialect/SV/hw-eliminate-inout-ports-errors.mlir new file mode 100644 index 000000000000..6257cbaca578 --- /dev/null +++ b/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) { + // expected-error @+1 {{uses hw.inout port "a" but the operation itself is unsupported.}} + "foo.bar" (%a) : (!hw.inout) -> () +} + +// ----- + +// expected-error @+1 {{multiple writers of inout port "a" is unsupported.}} +hw.module @multipleWriters(%a: !hw.inout) { + %0 = hw.constant 0 : i42 + sv.assign %a, %0 : i42 + sv.assign %a, %0 : i42 +} diff --git a/test/Dialect/SV/hw-eliminate-inout-ports.mlir b/test/Dialect/SV/hw-eliminate-inout-ports.mlir new file mode 100644 index 000000000000..a07e1b426afe --- /dev/null +++ b/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) -> (out: i42) { + %aget = sv.read_inout %a: !hw.inout + 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) { + %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) -> (out: i42) { + %aget = sv.read_inout %a: !hw.inout + sv.assign %a, %aget : i42 + hw.output %aget : i42 +} + +// CHECK-LABEL: hw.module @oneLevel() { +// CHECK: %[[VAL_0:.*]] = sv.wire : !hw.inout +// CHECK: %[[VAL_1:.*]] = sv.read_inout %[[VAL_0]] : !hw.inout +// 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 +// 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 + %read = hw.instance "read" @read(a : %0 : !hw.inout) -> (out: i42) + hw.instance "write" @write(a : %0 : !hw.inout) -> () + %read_write = hw.instance "readWrite" @read_write(a : %0 : !hw.inout) -> (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) -> () { + hw.instance "write" @write(a : %a : !hw.inout) -> () +} + +// CHECK-LABEL: hw.module @passthroughTwoLevels() { +// CHECK: %[[VAL_0:.*]] = sv.wire : !hw.inout +// 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 + hw.instance "passthrough" @passthrough(a : %0 : !hw.inout) -> () +} + + +// 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) { +// CHECK: %[[VAL_0:.*]] = sv.wire : !hw.inout +// CHECK: hw.output %[[VAL_0]] : !hw.inout +// CHECK: } +hw.module @outputInout() -> (out : !hw.inout) { + %0 = sv.wire : !hw.inout + hw.output %0 : !hw.inout +} + +// CHECK-LABEL: hw.module @outputInoutDriver() { +// CHECK: %[[VAL_0:.*]] = hw.instance "outputInout" @outputInout() -> (out: !hw.inout) +// 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) + hw.instance "write" @write(a : %0 : !hw.inout) -> () +}