From 0a3861792a2d245327415e06383f5dd06bfca25e Mon Sep 17 00:00:00 2001 From: Andrew Young Date: Fri, 2 Aug 2024 13:25:20 -0700 Subject: [PATCH] [FIRRTL] Add CheckRecursiveInstantiation diagnostic pass This adds a pass to FIRRTL to check for recursive module instantiation, which is illegal because it corresponds to infinitely sized hardware. This uses the path based strongly connected component (SCC) algorithm with some extra book keeping to track if there was a loop in the current SCC or not. This is especially helpful to detect if a single-element SCC actually contains a loop or not. This does not use the upstream SCCIterator because we hope that this version of the algorithm is faster by a constant factor, we don't need to visit the SCCs in post-order, and hopefully faster handling of cycles in single-element SCCs (which requires re-scanning all edges from a node using the upstream version). --- include/circt/Dialect/FIRRTL/Passes.h | 2 + include/circt/Dialect/FIRRTL/Passes.td | 14 +++ lib/Dialect/FIRRTL/Transforms/CMakeLists.txt | 1 + .../CheckRecursiveInstantiation.cpp | 96 +++++++++++++++++++ .../check-recursive-instantiation-errors.mlir | 70 ++++++++++++++ .../FIRRTL/check-recursive-instantiation.mlir | 6 ++ 6 files changed, 189 insertions(+) create mode 100644 lib/Dialect/FIRRTL/Transforms/CheckRecursiveInstantiation.cpp create mode 100644 test/Dialect/FIRRTL/check-recursive-instantiation-errors.mlir create mode 100644 test/Dialect/FIRRTL/check-recursive-instantiation.mlir diff --git a/include/circt/Dialect/FIRRTL/Passes.h b/include/circt/Dialect/FIRRTL/Passes.h index d3dd11735179..1ae1e07e5358 100644 --- a/include/circt/Dialect/FIRRTL/Passes.h +++ b/include/circt/Dialect/FIRRTL/Passes.h @@ -214,6 +214,8 @@ std::unique_ptr createLowerDPIPass(); std::unique_ptr createAssignOutputDirsPass(mlir::StringRef outputDir = ""); +std::unique_ptr createCheckRecursiveInstantiation(); + /// Generate the code for registering passes. #define GEN_PASS_REGISTRATION #include "circt/Dialect/FIRRTL/Passes.h.inc" diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 4af7367449b7..22c2207f47ad 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -968,4 +968,18 @@ def ProbesToSignals : Pass<"firrtl-probes-to-signals", "firrtl::CircuitOp"> { let constructor = "circt::firrtl::createProbesToSignalsPass()"; } +def CheckRecursiveInstantiation : Pass<"firrtl-check-recursive-instantiation", + "firrtl::CircuitOp"> { + let summary = "Check for illegal recursive instantiation"; + let description = [{ + This pass checks for illegal recursive module instantion. Recursive + instantiation is when a module instantiates itself, either directly or + indirectly through other modules it instantiates. Recursive module + instantiation is illegal because it would require infinite hardware to + synthesize. Recursive class instantiation is illegal as it would create an + infinite loop. + }]; + let constructor = "circt::firrtl::createCheckRecursiveInstantiation()"; +} + #endif // CIRCT_DIALECT_FIRRTL_PASSES_TD diff --git a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt index c885ea49e910..e86980a540a3 100755 --- a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt +++ b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt @@ -3,6 +3,7 @@ add_circt_dialect_library(CIRCTFIRRTLTransforms AddSeqMemPorts.cpp BlackBoxReader.cpp CheckCombLoops.cpp + CheckRecursiveInstantiation.cpp CreateCompanionAssume.cpp CreateSiFiveMetadata.cpp Dedup.cpp diff --git a/lib/Dialect/FIRRTL/Transforms/CheckRecursiveInstantiation.cpp b/lib/Dialect/FIRRTL/Transforms/CheckRecursiveInstantiation.cpp new file mode 100644 index 000000000000..59c44fd11f8b --- /dev/null +++ b/lib/Dialect/FIRRTL/Transforms/CheckRecursiveInstantiation.cpp @@ -0,0 +1,96 @@ +//===- CheckRecursiveInstantiation.cpp - Check recurisve instantiation ----===// +// +// 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/FIRRTLInstanceGraph.h" +#include "circt/Dialect/FIRRTL/Passes.h" +#include "mlir/Pass/Pass.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SCCIterator.h" + +namespace circt { +namespace firrtl { +#define GEN_PASS_DEF_CHECKRECURSIVEINSTANTIATION +#include "circt/Dialect/FIRRTL/Passes.h.inc" +} // namespace firrtl +} // namespace circt + +using namespace circt; +using namespace firrtl; + +static void printPath(InstanceGraph &instanceGraph, + ArrayRef nodes) { + assert(nodes.size() > 0 && "an scc should have at least one node"); + + // The first node in the SCC where we start the path from. + auto *first = nodes.front(); + auto diag = emitError(first->getModule().getLoc(), "recursive instantiation"); + + // A set of all nodes in the SCC for quick lookup. + llvm::SmallPtrSet scc(nodes.begin(), nodes.end()); + + // A set of all nodes seen in the path. + llvm::SmallPtrSet visited; + + // The current node we are examining. + auto *current = first; + while (true) { + // Iterate all instances in this module. + for (auto *record : *current) { + auto *target = record->getTarget(); + // Filter out the edges that don't point in the SCC. + if (!scc.contains(target)) + continue; + // If this node is already on our path, skip it. + if (visited.contains(target)) + continue; + auto ¬e = diag.attachNote(record->getInstance().getLoc()); + note << record->getParent()->getModule().getModuleName(); + note << " instantiates " + << record->getTarget()->getModule().getModuleName() << " here"; + // If we have looped back to the starting point, return here. + if (target == first) + return; + // Recurse on this edge. + current = target; + visited.insert(target); + break; + } + } +} + +static LogicalResult run(InstanceGraph &instanceGraph) { + bool error = false; + for (auto it = llvm::scc_begin(&instanceGraph), + end = llvm::scc_end(&instanceGraph); + it != end; ++it) { + if (it.hasCycle()) { + printPath(instanceGraph, *it); + error = true; + } + } + return failure(error); +} + +namespace { + +class CheckRecursiveInstantiationPass + : public impl::CheckRecursiveInstantiationBase< + CheckRecursiveInstantiationPass> { +public: + void runOnOperation() override { + auto &instanceGraph = getAnalysis(); + if (failed(run(instanceGraph))) + return signalPassFailure(); + markAllAnalysesPreserved(); + } // namespace +}; +} // namespace + +std::unique_ptr circt::firrtl::createCheckRecursiveInstantiation() { + return std::make_unique(); +} diff --git a/test/Dialect/FIRRTL/check-recursive-instantiation-errors.mlir b/test/Dialect/FIRRTL/check-recursive-instantiation-errors.mlir new file mode 100644 index 000000000000..4f85c7a59820 --- /dev/null +++ b/test/Dialect/FIRRTL/check-recursive-instantiation-errors.mlir @@ -0,0 +1,70 @@ +// RUN: circt-opt -pass-pipeline='builtin.module(firrtl.circuit(firrtl-check-recursive-instantiation))' %s --verify-diagnostics --split-input-file + +firrtl.circuit "SelfLoop0" { + // expected-error @below {{recursive instantiation}} + firrtl.module @SelfLoop0() { + // expected-note @below {{SelfLoop0 instantiates SelfLoop0 here}} + firrtl.instance inst @SelfLoop0() + } +} + +// ----- + +firrtl.circuit "SelfLoop1" { + // expected-error @below {{recursive instantiation}} + firrtl.module @SelfLoop1() { + // Should only print one of the instance paths. + // expected-note @below {{SelfLoop1 instantiates SelfLoop1 here}} + firrtl.instance inst @SelfLoop1() + firrtl.instance inst @SelfLoop1() + } +} + +// ----- + +firrtl.circuit "TwoLoops" { + // Should report both recursive module instantions. + // expected-error @below {{recursive instantiation}} + firrtl.module @TwoLoops() { + // expected-note @below {{TwoLoops instantiates TwoLoops here}} + firrtl.instance inst @TwoLoops() + firrtl.instance inst @OtherModule() + } + // expected-error @below {{recursive instantiation}} + firrtl.module @OtherModule() { + // expected-note @below {{OtherModule instantiates OtherModule here}} + firrtl.instance inst @OtherModule() + } +} + +// ----- + +firrtl.circuit "MutualLoop" { + firrtl.module @MutualLoop() { + firrtl.instance a @A() + } + firrtl.module @A() { + // expected-note @below {{A instantiates B here}} + firrtl.instance b @B() + } + // expected-error @below {{recursive instantiation}} + firrtl.module @B() { + // expected-note @below {{B instantiates A here}} + firrtl.instance a @A() + } +} + +// ----- + +// Should disallow recursive class instantiation. +firrtl.circuit "Classes" { + firrtl.module @Classes() { + firrtl.object @RecursiveClass() + } + + // expected-error @below {{recursive instantiation}} + firrtl.class @RecursiveClass() { + // expected-note @below {{RecursiveClass instantiates RecursiveClass here}} + %0 = firrtl.object @RecursiveClass() + } +} diff --git a/test/Dialect/FIRRTL/check-recursive-instantiation.mlir b/test/Dialect/FIRRTL/check-recursive-instantiation.mlir new file mode 100644 index 000000000000..c9ede2b247bb --- /dev/null +++ b/test/Dialect/FIRRTL/check-recursive-instantiation.mlir @@ -0,0 +1,6 @@ +// RUN: circt-opt -pass-pipeline='builtin.module(firrtl.circuit(firrtl-check-recursive-instantiation))' %s | FileCheck %s + +// CHECK-LABEL: firrtl.circuit "NoLoop" +firrtl.circuit "NoLoop" { + firrtl.module @NoLoop() { } +}