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

@@ -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
 ==========================

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<AstCell*> m_cellps;  // Cells left to process (in current module)
 
+    // Map from AstNodeModule to set of all AstNodeModules that instantiates it.
+    std::unordered_map<AstNodeModule*, std::unordered_set<AstNodeModule*>> 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<AstNodeModule*> 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);

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;

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