Revert: Fix --hierarchical parameter specialization name collisions (#8555 revert) (#8565).

This commit is contained in:
Wilson Snyder
2026-10-01 07:48:36 -04:00
parent 33a6d05382
commit e04f350109
9 changed files with 35 additions and 231 deletions
-1
View File
@@ -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]
+31 -33
View File
@@ -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 <deque>
#include <map>
#include <memory>
#include <set>
#include <unordered_map>
#include <vector>
@@ -281,7 +281,7 @@ class ParamProcessor final {
std::map<const std::string, std::string>
m_longMap; // Hash of very long names to unique identity number
std::set<std::string> 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<const std::string, std::string> m_valueNames; // Parameter value text to its name
std::set<std::string> 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<const V3Hash, int> 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<string>& 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;
}
+1 -1
View File
@@ -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;
| ^~~~~
-18
View File
@@ -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()
-111
View File
@@ -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
+1 -1
View File
@@ -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;
| ^~~~~~~~~~
-18
View File
@@ -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()
-46
View File
@@ -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
+2 -2
View File
@@ -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;
| ^~~