Keep recursive module list topologically (#3324).

Fixes (#3324).
2022-03-05 14:08:49 +00:00 · 2022-03-05 14:08:49 +00:00 · 3737d209f6
commit 3737d209f6
parent 29c4b0a141
8 changed files with 84 additions and 16 deletions
--- a/1
+++ b/1
@ -19,6 +19,7 @@ Verilator 4.219 devel
 * Fix skipping public enum values with four-state values (#3303).
 * Fix $readmem file not found to be warning not error (#3310). [Alexander Grobman]
 * Fix compile error with --trace-fst --sc (#3332). [leavinel]
+* Fix crash in recursive module inlining (#3324). [Larry Doolittle]


 Verilator 4.218 2022-01-17
--- a/src/V3Param.cpp
+++ b/src/V3Param.cpp
@ -554,10 +554,15 @@ class ParamProcessor final {
            cellp->v3error("Exceeded maximum --module-recursion-depth of "
                           << v3Global.opt.moduleRecursionDepth());
        }
-        // Keep tree sorted by level
+        // 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.
        AstNodeModule* insertp = srcModp;
        while (VN_IS(insertp->nextp(), NodeModule)
-               && VN_AS(insertp->nextp(), NodeModule)->level() < newmodp->level()) {
+               && VN_AS(insertp->nextp(), NodeModule)->level() <= newmodp->level()) {
            insertp = VN_AS(insertp->nextp(), NodeModule);
        }
        insertp->addNextHere(newmodp);
--- a/test_regress/t/t_lint_once_bad.out
+++ b/test_regress/t/t_lint_once_bad.out
@ -1,11 +1,11 @@
 %Warning-UNUSED: t/t_lint_once_bad.v:19:14: Signal is not driven, nor used: 'unus1'
-                                          : ... In instance t.sub3
+                                          : ... In instance t.sub1
   19 |    reg [A:0] unus1;    reg [A:0] unus2;
      |              ^~~~~
                 ... For warning description see https://verilator.org/warn/UNUSED?v=latest
                 ... Use "/* verilator lint_off UNUSED */" and lint_on around source to disable this message.
 %Warning-UNUSED: t/t_lint_once_bad.v:19:34: Signal is not driven, nor used: 'unus2'
-                                          : ... In instance t.sub3
+                                          : ... In instance t.sub1
   19 |    reg [A:0] unus1;    reg [A:0] unus2;
      |                                  ^~~~~
 %Error: Exiting due to
--- a/test_regress/t/t_lint_repeat_bad.out
+++ b/test_regress/t/t_lint_repeat_bad.out
@ -1,5 +1,5 @@
 %Warning-WIDTH: t/t_lint_repeat_bad.v:18:17: Operator ASSIGNW expects 1 bits on the Assign RHS, but Assign RHS's VARREF 'a' generates 2 bits.
-                                           : ... In instance t.sub2
+                                           : ... In instance t.sub3
   18 |    wire [0:0] b = a;
      |                 ^
                ... For warning description see https://verilator.org/warn/WIDTH?v=latest
--- a/test_regress/t/t_recursive_module_bug.pl
+++ b/test_regress/t/t_recursive_module_bug.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;
--- a/test_regress/t/t_recursive_module_bug.v
+++ b/test_regress/t/t_recursive_module_bug.v
@ -0,0 +1,46 @@
+// 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.
+// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+// This hits a case where parameter specialization of recursive modules
+// used to yield a module list that was not topologically sorted, which
+// then caused V3Inline to blow up as it assumes that.
+
+module top #(
+    parameter N=8
+) (
+    input   wire  [N-1:0] i,
+    output  wire  [N-1:0] o,
+    output  wire  [N-1:0] a
+);
+
+sub #(.N(N)) inst(.i(i), .o(a));
+
+generate if (N > 1) begin: recursive
+    top #(.N(N/2)) hi(.i(i[N   - 1:N/2]), .o(o[N   - 1:N/2]), .a());
+    top #(.N(N/2)) lo(.i(i[N/2 - 1:  0]), .o(o[N/2 - 1:  0]), .a());
+end else begin: base
+    assign o = i;
+end endgenerate
+
+endmodule
+
+module sub #(
+    parameter N = 8
+) (
+    input   wire  [N-1:0] i,
+    output  wire  [N-1:0] o
+);
+
+generate if (N > 1) begin: recursive
+    sub #(.N(N/2)) hi(.i(i[N   - 1:N/2]), .o(o[N   - 1:N/2]));
+    sub #(.N(N/2)) lo(.i(i[N/2 - 1:  0]), .o(o[N/2 - 1:  0]));
+end else begin: base
+    assign o = i;
+end endgenerate
+
+endmodule
--- a/test_regress/t/t_xml_first.out
+++ b/test_regress/t/t_xml_first.out
@ -45,6 +45,15 @@
        </port>
      </instance>
    </module>
+    <module loc="d,46,8,46,12" name="mod2" origName="mod2">
+      <var loc="d,48,10,48,13" name="clk" dtype_id="1" dir="input" pinIndex="1" vartype="logic" origName="clk"/>
+      <var loc="d,49,16,49,17" name="d" dtype_id="2" dir="input" pinIndex="2" vartype="logic" origName="d"/>
+      <var loc="d,50,22,50,23" name="q" dtype_id="2" dir="output" pinIndex="3" vartype="logic" origName="q"/>
+      <contassign loc="d,53,13,53,14" dtype_id="2">
+        <varref loc="d,53,15,53,16" name="d" dtype_id="2"/>
+        <varref loc="d,53,11,53,12" name="q" dtype_id="2"/>
+      </contassign>
+    </module>
    <module loc="d,31,8,31,12" name="mod1__W4" origName="mod1">
      <var loc="d,32,15,32,20" name="WIDTH" dtype_id="3" vartype="logic" origName="WIDTH" param="true">
        <const loc="d,19,18,19,19" name="32&apos;sh4" dtype_id="3"/>
@ -67,17 +76,8 @@
        </assigndly>
      </always>
    </module>
-    <module loc="d,46,8,46,12" name="mod2" origName="mod2">
-      <var loc="d,48,10,48,13" name="clk" dtype_id="1" dir="input" pinIndex="1" vartype="logic" origName="clk"/>
-      <var loc="d,49,16,49,17" name="d" dtype_id="2" dir="input" pinIndex="2" vartype="logic" origName="d"/>
-      <var loc="d,50,22,50,23" name="q" dtype_id="2" dir="output" pinIndex="3" vartype="logic" origName="q"/>
-      <contassign loc="d,53,13,53,14" dtype_id="2">
-        <varref loc="d,53,15,53,16" name="d" dtype_id="2"/>
-        <varref loc="d,53,11,53,12" name="q" dtype_id="2"/>
-      </contassign>
-    </module>
    <typetable loc="a,0,0,0,0">
-      <basicdtype loc="d,48,10,48,13" id="1" name="logic"/>
+      <basicdtype loc="d,34,24,34,27" id="1" name="logic"/>
      <basicdtype loc="d,14,10,14,11" id="2" name="logic" left="3" right="0"/>
      <basicdtype loc="d,19,18,19,19" id="3" name="logic" left="31" right="0" signed="true"/>
    </typetable>
--- a/test_regress/t/t_xml_flat.out
+++ b/test_regress/t/t_xml_flat.out
@ -106,7 +106,7 @@
      </topscope>
    </module>
    <typetable loc="a,0,0,0,0">
-      <basicdtype loc="d,48,10,48,13" id="1" name="logic"/>
+      <basicdtype loc="d,34,24,34,27" id="1" name="logic"/>
      <basicdtype loc="d,14,10,14,11" id="2" name="logic" left="3" right="0"/>
      <basicdtype loc="d,19,18,19,19" id="3" name="logic" left="31" right="0" signed="true"/>
    </typetable>