From 3737d209f619597942b642d06ee26d663aa35617 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 5 Mar 2022 14:08:49 +0000 Subject: [PATCH] Keep recursive module list topologically (#3324). Fixes (#3324). --- Changes | 1 + src/V3Param.cpp | 9 +++-- test_regress/t/t_lint_once_bad.out | 4 +-- test_regress/t/t_lint_repeat_bad.out | 2 +- test_regress/t/t_recursive_module_bug.pl | 16 +++++++++ test_regress/t/t_recursive_module_bug.v | 46 ++++++++++++++++++++++++ test_regress/t/t_xml_first.out | 20 +++++------ test_regress/t/t_xml_flat.out | 2 +- 8 files changed, 84 insertions(+), 16 deletions(-) create mode 100755 test_regress/t/t_recursive_module_bug.pl create mode 100644 test_regress/t/t_recursive_module_bug.v diff --git a/Changes b/Changes index a99573c83..682450cc4 100644 --- a/Changes +++ b/Changes @@ -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 diff --git a/src/V3Param.cpp b/src/V3Param.cpp index 30445b787..cdb4ce9a2 100644 --- 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); diff --git a/test_regress/t/t_lint_once_bad.out b/test_regress/t/t_lint_once_bad.out index 4a8917764..04dca3c21 100644 --- 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 diff --git a/test_regress/t/t_lint_repeat_bad.out b/test_regress/t/t_lint_repeat_bad.out index 2d8287e78..35067222b 100644 --- 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 diff --git a/test_regress/t/t_recursive_module_bug.pl b/test_regress/t/t_recursive_module_bug.pl new file mode 100755 index 000000000..2ef6db6a2 --- /dev/null +++ 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; diff --git a/test_regress/t/t_recursive_module_bug.v b/test_regress/t/t_recursive_module_bug.v new file mode 100644 index 000000000..00d362073 --- /dev/null +++ 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 diff --git a/test_regress/t/t_xml_first.out b/test_regress/t/t_xml_first.out index 51bf2a9aa..50968cab7 100644 --- a/test_regress/t/t_xml_first.out +++ b/test_regress/t/t_xml_first.out @@ -45,6 +45,15 @@ + + + + + + + + + @@ -67,17 +76,8 @@ - - - - - - - - - - + diff --git a/test_regress/t/t_xml_flat.out b/test_regress/t/t_xml_flat.out index fac6779bb..eae232855 100644 --- a/test_regress/t/t_xml_flat.out +++ b/test_regress/t/t_xml_flat.out @@ -106,7 +106,7 @@ - +