From e04f3501095b64f4b67b3b0b21e8e4aec5f9ffd7 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Thu, 1 Oct 2026 07:48:36 -0400 Subject: [PATCH] Revert: Fix --hierarchical parameter specialization name collisions (#8555 revert) (#8565). --- Changes | 1 - src/V3Param.cpp | 64 ++++++------- test_regress/t/t_class_param_enum_bad.out | 2 +- test_regress/t/t_hier_block_param_name.py | 18 ---- test_regress/t/t_hier_block_param_name.v | 111 ---------------------- test_regress/t/t_mailbox_bad.out | 2 +- test_regress/t/t_param_array_sparse.py | 18 ---- test_regress/t/t_param_array_sparse.v | 46 --------- test_regress/t/t_timing_at_dtype_bad.out | 4 +- 9 files changed, 35 insertions(+), 231 deletions(-) delete mode 100755 test_regress/t/t_hier_block_param_name.py delete mode 100644 test_regress/t/t_hier_block_param_name.v delete mode 100755 test_regress/t/t_param_array_sparse.py delete mode 100644 test_regress/t/t_param_array_sparse.v diff --git a/Changes b/Changes index f3ede3625..db12ebcd3 100644 --- a/Changes +++ b/Changes @@ -154,7 +154,6 @@ Verilator 5.053 devel * Fix stack depth calculations over calls (#8544). [Nick Brereton] * Fix cascading errors after unsupported class dot reference in parameter value (#8551). [Martijn Wobbes, Antmicro Ltd.] * Fix vpi_put_value crash on NULL value pointer (#8553). [Nick Brereton] -* Fix --hierarchical parameter specialization name collisions (#8555). [Marco Bartoli] * Fix dehash of a suffix after a hashed name (#8556). [Ethan Sifferman] diff --git a/src/V3Param.cpp b/src/V3Param.cpp index a5e9794d9..d1ffeef34 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -57,6 +57,7 @@ #include "V3Case.h" #include "V3Const.h" #include "V3EmitV.h" +#include "V3Hasher.h" #include "V3LinkDotIfaceCapture.h" #include "V3MemberMap.h" #include "V3Os.h" @@ -71,7 +72,6 @@ #include #include #include -#include #include #include @@ -281,7 +281,7 @@ class ParamProcessor final { std::map m_longMap; // Hash of very long names to unique identity number - std::set m_longSuffixes; // Digest suffixes used by m_longMap + int m_longId = 0; // All module names that are loaded from source code // Generated modules by this visitor is not included @@ -290,13 +290,8 @@ class ParamProcessor final { CloneMap m_originalParams; // Map between parameters of copied parameteized classes and their // original nodes - std::map m_valueNames; // Parameter value text to its name - std::set m_valueSuffixes; // Digest suffixes used by m_valueNames - // Number of digest hex digits in a name. With 8 digits, a collision between names made in - // different runs, such as those of different hierarchical blocks, is unlikely. Within one - // run, digestSuffix resolves a collision by lengthening the later text's suffix, which can - // then differ between runs. - static constexpr std::string::size_type DIGEST_DIGITS = 8; + std::map m_valueMap; // Hash of node hash to param value + int m_nextValue = 1; // Next value to use in m_valueMap const AstNodeModule* m_modp = nullptr; // Current module being processed @@ -397,12 +392,11 @@ class ParamProcessor final { key += "] "; key += paramValueString(dtypep->subDTypep()); } else if (const AstInitArray* const initp = VN_CAST(nodep, InitArray)) { - // Include the indices and the default, as with a default the map may be sparse key += "{"; - for (const auto& it : initp->map()) { - key += cvtToStr(it.first) + ":" + paramValueString(it.second->valuep()) + ","; + for (auto it : initp->map()) { + key += paramValueString(it.second->valuep()); + key += ","; } - if (initp->defaultp()) key += "default:" + paramValueString(initp->defaultp()) + ","; key += "}"; } else if (const AstConsPackUOrStruct* const structp = VN_CAST(nodep, ConsPackUOrStruct)) { key += "{"; @@ -452,39 +446,43 @@ class ParamProcessor final { return key; } - // Return a name suffix for 'text' from its SHA-512 digest. Hierarchical blocks are - // Verilated in separate runs, where a counter would restart, but the digest is the same in - // every run. Use the shortest digest prefix of at least DIGEST_DIGITS digits that is not - // already in 'usedr', and add it to 'usedr'. - static string digestSuffix(const string& text, std::set& usedr) { - const string hex = VHashSha512{text}.digestHex(); - // Force collisions of the prefixes -- for testing only - string::size_type digits = v3Global.opt.debugCollision() ? 1 : DIGEST_DIGITS; - while (digits < hex.size() && !usedr.insert(hex.substr(0, digits)).second) ++digits; - return hex.substr(0, digits); - } string paramValueNumber(AstNode* nodep) { + // For type parameters (NodeDType), use only the string representation for hashing. + // Using V3Hasher::uncachedHash includes AST node pointer which differs for equivalent + // types represented by different AST nodes (e.g., parameterized class specializations). + // For value parameters, we can still use the AST hash for better collision resistance. // All call sites resolve through skipRefToNonRefp() or pass non-DType // nodes, so nodep should never be a bare RefDType here. if (VN_IS(nodep, RefDType)) { // LCOV_EXCL_LINE nodep->v3fatalSrc("Unexpected RefDType in paramValueNumber"); // LCOV_EXCL_LINE } - // Name the value by its text, which is the same for equal values or types in every run. - // V3Hasher is unsuitable, as it hashes node pointers, which can differ for equal types. - // V3Hash of a string is unsuitable, as std::hash varies between C++ libraries. - const string text = paramValueString(nodep); - const auto pair = m_valueNames.emplace(text, ""); - if (pair.second) pair.first->second = "z" + digestSuffix(text, m_valueSuffixes); - return pair.first->second; + const string paramStr = paramValueString(nodep); + V3Hash hash; + if (VN_IS(nodep, NodeDType)) { + // Type parameter: use only string-based hash for type equivalence + hash = V3Hash{paramStr}; + } else { + // Value parameter: use AST hash + string for better collision resistance + hash = V3Hasher::uncachedHash(nodep) + paramStr; + } + // Force hash collisions -- for testing only + // cppcheck-suppress unreadVariable + if (VL_UNLIKELY(v3Global.opt.debugCollision())) hash = V3Hash{paramStr}; + int num; + const auto pair = m_valueMap.emplace(hash, 0); + if (pair.second) pair.first->second = m_nextValue++; + num = pair.first->second; + return "z"s + cvtToStr(num); } string moduleCalcName(const AstNodeModule* srcModp, const string& longname) { string newname = longname; if (longname.length() > 30) { const auto pair = m_longMap.emplace(longname, ""); if (pair.second) { + newname = srcModp->name(); // We use all upper case above, so lower here can't conflict - pair.first->second - = srcModp->name() + "__pi" + digestSuffix(longname, m_longSuffixes); + newname += "__pi" + cvtToStr(++m_longId); + pair.first->second = newname; } newname = pair.first->second; } diff --git a/test_regress/t/t_class_param_enum_bad.out b/test_regress/t/t_class_param_enum_bad.out index ff73ba725..1bd914c75 100644 --- a/test_regress/t/t_class_param_enum_bad.out +++ b/test_regress/t/t_class_param_enum_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_class_param_enum_bad.v:25:40: Assign RHS expects a CLASSREFDTYPE 'Converter__Tz7d09f080', got CLASSREFDTYPE 'Converter__Tza32c8cf8' +%Error: t/t_class_param_enum_bad.v:25:40: Assign RHS expects a CLASSREFDTYPE 'Converter__Tz2', got CLASSREFDTYPE 'Converter__Tz1' : ... note: In instance 't' 25 | automatic Converter #(bit) conv2 = conv1; | ^~~~~ diff --git a/test_regress/t/t_hier_block_param_name.py b/test_regress/t/t_hier_block_param_name.py deleted file mode 100755 index 6042640d3..000000000 --- a/test_regress/t/t_hier_block_param_name.py +++ /dev/null @@ -1,18 +0,0 @@ -#!/usr/bin/env python3 -# DESCRIPTION: Verilator: Verilog Test driver/expect definition -# -# 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-FileCopyrightText: 2026 Wilson Snyder -# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 - -import vltest_bootstrap - -test.scenarios('vlt') - -test.compile(verilator_flags2=['--binary', '--hierarchical']) - -test.execute() - -test.passes() diff --git a/test_regress/t/t_hier_block_param_name.v b/test_regress/t/t_hier_block_param_name.v deleted file mode 100644 index 05e555cd6..000000000 --- a/test_regress/t/t_hier_block_param_name.v +++ /dev/null @@ -1,111 +0,0 @@ -// DESCRIPTION: Verilator: Verilog Test module -// -// This file ONLY is placed under the Creative Commons Public Domain. -// SPDX-FileCopyrightText: 2026 Wilson Snyder -// SPDX-License-Identifier: CC0-1.0 - -// Specializations of a parameterized class are distinct types, but for matching ones, and -// $typename returns their names (IEEE 1800-2023 8.25, 20.6.1). Hierarchical block 'hb' and the -// parent are Verilated in runs of their own, which each name the specializations they elaborate: -// a specialization has one name in both, and distinct ones distinct names. hb elaborates two -// specializations of each class, and passes their names to the parent, which elaborates the -// second of those. - -// verilog_format: off -`define stop $stop -`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); -`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got='%s' exp='%s'\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); -// verilog_format: on - -// A type name as characters, the first the most significant, for a port to pass -localparam int NAME_CHARS = 64; - -function automatic logic [8*NAME_CHARS-1:0] to_bits(string name); - to_bits = '0; - for (int i = 0; i < name.len() && i < NAME_CHARS; ++i) to_bits[8*(NAME_CHARS-1-i)+:8] = name[i]; -endfunction - -function automatic string to_name(logic [8*NAME_CHARS-1:0] name_bits); - to_name = ""; - for (int i = 0; i < NAME_CHARS; ++i) begin - if (name_bits[8*(NAME_CHARS-1-i)+:8] == 0) break; - to_name = {to_name, string'(name_bits[8*(NAME_CHARS-1-i)+:8])}; - end -endfunction - -// Of a type parameter -class TypeParam #( - type T = int -); - T v; -endclass - -// Of a parameter not of 32 bits -class ByteParam #( - bit [7:0] B = 8'd1 -); - bit [7:0] v = B; -endclass - -// Of parameters whose values make a long name -class LongParameters #( - int FIRST_PARAMETER = 1, - int SECOND_PARAMETER = 2 -); - int v = FIRST_PARAMETER + SECOND_PARAMETER; -endclass - -module hb ( - output logic [8*NAME_CHARS-1:0] type_a_name, - output logic [8*NAME_CHARS-1:0] type_b_name, - output logic [8*NAME_CHARS-1:0] byte_a_name, - output logic [8*NAME_CHARS-1:0] byte_b_name, - output logic [8*NAME_CHARS-1:0] long_a_name, - output logic [8*NAME_CHARS-1:0] long_b_name -); - /*verilator hier_block*/ - TypeParam #(byte) type_a; - TypeParam #(shortint) type_b; - ByteParam #(8'd2) byte_a; - ByteParam #(8'd3) byte_b; - LongParameters #(1111111, 2222222) long_a; - LongParameters #(3333333, 4444444) long_b; - - // In a procedure, as the handles are dynamic (IEEE 1800-2023 6.21) - initial begin - type_a_name = to_bits($typename(type_a)); - type_b_name = to_bits($typename(type_b)); - byte_a_name = to_bits($typename(byte_a)); - byte_b_name = to_bits($typename(byte_b)); - long_a_name = to_bits($typename(long_a)); - long_b_name = to_bits($typename(long_b)); - end -endmodule - -module t; - logic [8*NAME_CHARS-1:0] type_a_name; - logic [8*NAME_CHARS-1:0] type_b_name; - logic [8*NAME_CHARS-1:0] byte_a_name; - logic [8*NAME_CHARS-1:0] byte_b_name; - logic [8*NAME_CHARS-1:0] long_a_name; - logic [8*NAME_CHARS-1:0] long_b_name; - TypeParam #(shortint) type_b; - ByteParam #(8'd3) byte_b; - LongParameters #(3333333, 4444444) long_b; - - hb h (.*); - - // Without $finish, which could come before hb's names: the simulation ends, and runs the final - // blocks, once time zero has run - final begin - // A specialization of both has one name in both - `checks(to_name(type_b_name), $typename(type_b)); - `checks(to_name(byte_b_name), $typename(byte_b)); - `checks(to_name(long_b_name), $typename(long_b)); - // Distinct specializations have distinct names - `checkd(to_name(type_a_name) != $typename(type_b), 1); - `checkd(to_name(byte_a_name) != $typename(byte_b), 1); - `checkd(to_name(long_a_name) != $typename(long_b), 1); - $write("*-* All Finished *-*\n"); - end -endmodule diff --git a/test_regress/t/t_mailbox_bad.out b/test_regress/t/t_mailbox_bad.out index 8e638ed3c..e0d32b705 100644 --- a/test_regress/t/t_mailbox_bad.out +++ b/test_regress/t/t_mailbox_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_mailbox_bad.v:12:11: Class method 'bad_method' not found in class 'mailbox__Tz6d8d043b' +%Error: t/t_mailbox_bad.v:12:11: Class method 'bad_method' not found in class 'mailbox__Tz1' : ... note: In instance 't' 12 | if (m.bad_method() != 0) $stop; | ^~~~~~~~~~ diff --git a/test_regress/t/t_param_array_sparse.py b/test_regress/t/t_param_array_sparse.py deleted file mode 100755 index 46d1fe4c0..000000000 --- a/test_regress/t/t_param_array_sparse.py +++ /dev/null @@ -1,18 +0,0 @@ -#!/usr/bin/env python3 -# DESCRIPTION: Verilator: Verilog Test driver/expect definition -# -# 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-FileCopyrightText: 2026 Wilson Snyder -# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 - -import vltest_bootstrap - -test.scenarios('simulator') - -test.compile(verilator_flags2=['--binary']) - -test.execute() - -test.passes() diff --git a/test_regress/t/t_param_array_sparse.v b/test_regress/t/t_param_array_sparse.v deleted file mode 100644 index 866ed4720..000000000 --- a/test_regress/t/t_param_array_sparse.v +++ /dev/null @@ -1,46 +0,0 @@ -// DESCRIPTION: Verilator: Verilog Test module -// -// This file ONLY is placed under the Creative Commons Public Domain. -// SPDX-FileCopyrightText: 2026 Wilson Snyder -// SPDX-License-Identifier: CC0-1.0 - -// Unpacked array parameter values with equal elements at different indices are different -// values, so the instances given them must not share a module specialization. - -// verilog_format: off -`define stop $stop -`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); -// verilog_format: on - -typedef int arr_t[4]; - -// Sets one element only, so the constant value has that element and a default for the others -function automatic arr_t one_hot(int idx); - one_hot[idx] = 5; -endfunction - -module sub #( - parameter arr_t ARR = '{default: 0} -) ( - output arr_t o -); - assign o = ARR; -endmodule - -module t; - arr_t o1; - arr_t o2; - - sub #(.ARR(one_hot(1))) u1 (.o(o1)); - sub #(.ARR(one_hot(2))) u2 (.o(o2)); - - // Without $finish, which could come before the outputs: the simulation ends, and runs the final - // blocks, once time zero has run - final begin - `checkd(o1[1], 5); - `checkd(o1[2], 0); - `checkd(o2[1], 0); - `checkd(o2[2], 5); - $write("*-* All Finished *-*\n"); - end -endmodule diff --git a/test_regress/t/t_timing_at_dtype_bad.out b/test_regress/t/t_timing_at_dtype_bad.out index 121cba106..b4eb22942 100644 --- a/test_regress/t/t_timing_at_dtype_bad.out +++ b/test_regress/t/t_timing_at_dtype_bad.out @@ -1,11 +1,11 @@ %Error-UNSUPPORTED: t/t_timing_at_dtype_bad.v:20:12: Unsupported: Cannot detect changes on expression of complex type 'int$[$]' - : ... note: In instance 't::any_monitor__pif994bd0b' + : ... note: In instance 't::any_monitor__Tz1_TBz1' : ... May be caused by combinational cycles reported with UNOPTFLAT 20 | @req; | ^~~ ... For error description see https://verilator.org/warn/UNSUPPORTED?v=latest %Error-UNSUPPORTED: t/t_timing_at_dtype_bad.v:23:12: Unsupported: Cannot detect changes on expression of complex type 'int$[$]' - : ... note: In instance 't::any_monitor__pif994bd0b' + : ... note: In instance 't::any_monitor__Tz1_TBz1' : ... May be caused by combinational cycles reported with UNOPTFLAT 23 | @rsp; | ^~~