From 0b74e9b354057d09f57f6969bfe0d6b08af04bb9 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 23 Apr 2022 13:06:20 +0100 Subject: [PATCH] Ensure topological ordering of module list. At the end of V3Param, fix up the module list to be topologically sorted. We need to do this at the end as a later instantiation of a recursive module might instantiate an earlier specialization, which we cannot know until we processed everything. The rest of the compiler depends on the module list being topologically sorted. Fixes #3393 --- Changes | 1 + src/V3Param.cpp | 67 ++++++++++++++++++---- test_regress/t/t_recursive_module_bug_2.pl | 16 ++++++ test_regress/t/t_recursive_module_bug_2.v | 21 +++++++ 4 files changed, 95 insertions(+), 10 deletions(-) create mode 100755 test_regress/t/t_recursive_module_bug_2.pl create mode 100644 test_regress/t/t_recursive_module_bug_2.v diff --git a/Changes b/Changes index 12f8458f1..8a8a34272 100644 --- a/Changes +++ b/Changes @@ -21,6 +21,7 @@ Verilator 4.221 devel * Fix tracing interfaces inside interfaces (#3309). [Kevin Millis] * Fix filenames with dots overwriting debug .vpp files (#3373). * Fix including VK_USER_OBJS in make library (#3370). [Julien Margetts] +* Fix crash in recursive module inlining (#3393). [david-sawatzke] Verilator 4.220 2022-03-12 ========================== diff --git a/src/V3Param.cpp b/src/V3Param.cpp index bfad0f84c..607639ed9 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -555,14 +555,12 @@ class ParamProcessor final { cellp->v3error("Exceeded maximum --module-recursion-depth of " << v3Global.opt.moduleRecursionDepth()); } - // Keep tree sorted by level. Append to end of sub-list at the same level. This is - // important because due to the way recursive modules are handled, different - // parametrizations of the same recursive module end up with the same level (which in - // itself is a bit unfortunate). Nevertheless, as a later parametrization must not be above - // an earlier parametrization of a recursive module, it is sufficient to add to the end of - // the sub-list to keep the modules topologically sorted. + // Keep tree sorted by level. Note: Different parametrizations of the same recursive module + // end up with the same level, which we will need to fix up at the end, as we do not know + // up front how recursive modules are expanded, and a later expansion might re-use an + // earlier expansion (see t_recursive_module_bug_2). AstNodeModule* insertp = srcModp; - while (VN_IS(insertp->nextp(), NodeModule) + while (insertp->nextp() && VN_AS(insertp->nextp(), NodeModule)->level() <= newmodp->level()) { insertp = VN_AS(insertp->nextp(), NodeModule); } @@ -843,6 +841,9 @@ public: // Process parameter visitor class ParamVisitor final : public VNVisitor { + // NODE STATE + // AstNodeModule::user1 -> bool: already fixed level + // STATE ParamProcessor m_processor; // De-parameterize a cell, build modules UnrollStateful m_unroller; // Loop unroller @@ -852,6 +853,9 @@ class ParamVisitor final : public VNVisitor { string m_unlinkedTxt; // Text for AstUnlinkedRef std::deque m_cellps; // Cells left to process (in current module) + // Map from AstNodeModule to set of all AstNodeModules that instantiates it. + std::unordered_map> m_parentps; + // METHODS VL_DEBUG_FUNC; // Declare debug() @@ -900,6 +904,9 @@ class ParamVisitor final : public VNVisitor { // Add the (now potentially specialized) child module to the work queue workQueue.emplace(cellp->modp()->level(), cellp->modp()); + + // Add to the hierarchy registry + m_parentps[cellp->modp()].insert(modp); } } m_cellps.clear(); @@ -908,6 +915,18 @@ class ParamVisitor final : public VNVisitor { m_iterateModule = false; } + // Fix up level of module, based on who instantiates it + void fixLevel(AstNodeModule* modp) { + if (modp->user1SetOnce()) return; // Already fixed + if (m_parentps[modp].empty()) return; // Leave top levels alone + int maxParentLevel = 0; + for (AstNodeModule* parentp : m_parentps[modp]) { + fixLevel(parentp); // Ensure parent level is correct + maxParentLevel = std::max(maxParentLevel, parentp->level()); + } + if (modp->level() <= maxParentLevel) modp->level(maxParentLevel + 1); + } + // VISITORS virtual void visit(AstNodeModule* nodep) override { if (nodep->recursiveClone()) nodep->dead(true); // Fake, made for recursive elimination @@ -1191,10 +1210,38 @@ class ParamVisitor final : public VNVisitor { public: // CONSTRUCTORS - explicit ParamVisitor(AstNetlist* nodep) - : m_processor{nodep} { + explicit ParamVisitor(AstNetlist* netlistp) + : m_processor{netlistp} { // Relies on modules already being in top-down-order - iterate(nodep); + iterate(netlistp); + + // Re-sort module list to be in topological order and fix-up incorrect levels. We need to + // do this globally at the end due to the presence of recursive modules, which might be + // expanded in orders that reuse earlier specializations later at a lower level. + { + // Gather modules + std::vector modps; + for (AstNodeModule *modp = netlistp->modulesp(), *nextp; modp; modp = nextp) { + nextp = VN_AS(modp->nextp(), NodeModule); + modp->unlinkFrBack(); + modps.push_back(modp); + } + + // Fix-up levels + { + const VNUser1InUse user1InUse; + for (AstNodeModule* const modp : modps) fixLevel(modp); + } + + // Sort by level + std::stable_sort(modps.begin(), modps.end(), + [](const AstNodeModule* ap, const AstNodeModule* bp) { + return ap->level() < bp->level(); + }); + + // Re-insert modules + for (AstNodeModule* const modp : modps) netlistp->addModulep(modp); + } } virtual ~ParamVisitor() override = default; VL_UNCOPYABLE(ParamVisitor); diff --git a/test_regress/t/t_recursive_module_bug_2.pl b/test_regress/t/t_recursive_module_bug_2.pl new file mode 100755 index 000000000..2ef6db6a2 --- /dev/null +++ b/test_regress/t/t_recursive_module_bug_2.pl @@ -0,0 +1,16 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 by Geza Lore. This program is free software; you can +# redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(simulator => 1); + +compile(); + +ok(1); +1; diff --git a/test_regress/t/t_recursive_module_bug_2.v b/test_regress/t/t_recursive_module_bug_2.v new file mode 100644 index 000000000..fd47d945b --- /dev/null +++ b/test_regress/t/t_recursive_module_bug_2.v @@ -0,0 +1,21 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// Copyright 2022 by Geza Lore. This program is free software; you can +// redistribute it and/or modify it under the terms of either the GNU +// Lesser General Public License Version 3 or the Perl Artistic License +// Version 2.0. + +module a #(parameter N) (); + generate if (N > 1) begin + // With N == 5, this will first expand N == 2, then expand N == 3, + // which instantiates N == 2. This requires fixing up topological order + // in V3Param. + a #(.N( N/2)) sub_lo(); + a #(.N(N-N/2)) sub_hi(); + end + endgenerate +endmodule + +module top(); + a #(.N(5)) root (); +endmodule