From 5b280c1911d269156b20610478b09aeda2395cce Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Sun, 3 Jan 2021 12:19:37 +0900 Subject: [PATCH] Fix hierarchical verilation with explicit setting of default parameter value (#2738) * Test hierarchical block that is explicitly set its default parameter value. * Fix hierarchical verilation when a hierarchical block is instantiated with explicit setting of the default value. Parameterized hierarchical block must have mangled name even when all parameters have default value, otherwise the parameterized module will be hidden by protect-lib wrapper. * rename variable names. No functional change is intended. --- src/V3Param.cpp | 125 ++++++++++++++++++++++----- test_regress/t/t_hier_block.pl | 2 +- test_regress/t/t_hier_block.v | 20 +++++ test_regress/t/t_hier_block_cmake.pl | 2 +- test_regress/t/t_hier_block_sc.pl | 2 +- test_regress/t/t_hier_block_vlt.pl | 2 +- 6 files changed, 126 insertions(+), 27 deletions(-) diff --git a/src/V3Param.cpp b/src/V3Param.cpp index 7cced105d..89df4b371 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -71,7 +71,9 @@ class ParameterizedHierBlocks final { typedef HierBlockOptsByOrigName::const_iterator HierMapIt; typedef std::map HierBlockModMap; typedef std::map> ParamConstMap; + typedef std::map GParamsMap; // key:parameter name value:parameter typedef std::map HierParamsMap; + typedef std::map ModParamsMap; // key:module name // MEMBERS // key:Original module name, value:HiearchyBlockOption* @@ -82,6 +84,7 @@ class ParameterizedHierBlocks final { HierBlockModMap m_hierBlockMod; // Overridden parameters of the hierarchical block HierParamsMap m_hierParams; + ModParamsMap m_modParams; // Parameter variables of hierarchical blocks // METHODS VL_DEBUG_FUNC; // Declare debug() @@ -101,12 +104,23 @@ public: const bool inserted = consts.emplace(pIt->first, std::move(constp)).second; UASSERT(inserted, pIt->first << " is already added"); } + // origName may be already registered, but it's fine. + m_modParams.insert({hierOpt.second.origName(), {}}); } for (AstNodeModule* modp = nodep->modulesp(); modp; modp = VN_CAST(modp->nextp(), NodeModule)) { if (hierOpts.find(modp->prettyName()) != hierOpts.end()) { m_hierBlockMod.emplace(modp->name(), modp); } + const auto defParamIt = m_modParams.find(modp->name()); + if (defParamIt != m_modParams.end()) { + // modp is the original of parameterized hierarchical block + for (AstNode* stmtp = modp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { + if (AstVar* varp = VN_CAST(stmtp, Var)) { + if (varp->isGParam()) defParamIt->second.emplace(varp->name(), varp); + } + } + } } } AstNodeModule* findByParams(const string& origName, AstPin* firstPinp, @@ -117,6 +131,8 @@ public: // This module is a hierarchical block. Need to replace it by the protect-lib wrapper. const std::pair candidates = m_hierBlockOptsByOrigName.equal_range(origName); + const auto paramsIt = m_modParams.find(origName); + UASSERT_OBJ(paramsIt != m_modParams.end(), modp, origName << " must be registered"); HierMapIt hierIt; for (hierIt = candidates.first; hierIt != candidates.second; ++hierIt) { bool found = true; @@ -131,6 +147,14 @@ public: AstConst* constp = VN_CAST(pinp->exprp(), Const); UASSERT_OBJ(constp, pinp, "parameter for a hierarchical block must have been constified"); + const auto paramIt = paramsIt->second.find(modvarp->name()); + UASSERT_OBJ(paramIt != paramsIt->second.end(), modvarp, "must be registered"); + AstConst* defValuep = VN_CAST(paramIt->second->valuep(), Const); + if (defValuep && areSame(constp, defValuep)) { + UINFO(5, "Setting default value of " << constp << " to " << modvarp + << std::endl); + continue; // Skip this parameter because setting the same value + } const auto pIt = vlstd::as_const(params).find(modvarp->name()); UINFO(5, "Comparing " << modvarp->name() << " " << constp << std::endl); if (pIt == params.end() || paramIdx >= params.size() @@ -239,6 +263,10 @@ class ParamProcessor final { // Database to get protect-lib wrapper that matches parameters in hierarchical Verilation ParameterizedHierBlocks m_hierBlocks; + // Default parameter values key:parameter name, value:default value (can be nullptr) + typedef std::map DefaultValueMap; + // Default parameter values of hierarchical blocks + std::map m_defaultParameterValues; // METHODS VL_DEBUG_FUNC; // Declare debug() @@ -302,20 +330,16 @@ class ParamProcessor final { } return string("z") + cvtToStr(num); } - string moduleCalcName(AstNodeModule* srcModp, AstPin* paramsp, const string& longname) { + string moduleCalcName(AstNodeModule* srcModp, const string& longname) { string newname = longname; - if (longname.length() > 30 || srcModp->hierBlock()) { + if (longname.length() > 30) { const auto iter = m_longMap.find(longname); if (iter != m_longMap.end()) { newname = iter->second; } else { - if (srcModp->hierBlock()) { - newname = parametrizedHierBlockName(srcModp, paramsp); - } else { - newname = srcModp->name(); - // We use all upper case above, so lower here can't conflict - newname += "__pi" + cvtToStr(++m_longId); - } + newname = srcModp->name(); + // We use all upper case above, so lower here can't conflict + newname += "__pi" + cvtToStr(++m_longId); m_longMap.emplace(longname, newname); } } @@ -416,17 +440,61 @@ class ParamProcessor final { return false; } - string parametrizedHierBlockName(const AstNodeModule* modp, AstPin* paramPinsp) const { - VHashSha256 hash; - // Calculate hash using module name, parameter name, and parameter value - // The hash is used as the module suffix to find a module name that is unique in the design - hash.insert(modp->name()); + string parameterizedHierBlockName(AstNodeModule* modp, AstPin* paramPinsp) { + // Create a unique name in the following steps + // - Make a long name that includes all parameters, that appear + // in the alphabetical order. + // - Hash the long name to get valid Verilog symbol + UASSERT_OBJ(modp->hierBlock(), modp, "should be used for hierarchical block"); + + std::map pins; for (AstPin* pinp = paramPinsp; pinp; pinp = VN_CAST(pinp->nextp(), Pin)) { - if (AstVar* varp = pinp->modVarp()) hash.insert(varp->name()); - if (AstConst* constp = VN_CAST(pinp->exprp(), Const)) { - hash.insert(constp->num().ascii(false)); + checkSupportedParam(modp, pinp); + if (AstVar* varp = pinp->modVarp()) { + if (!pinp->exprp()) continue; + if (varp->isGParam()) { + AstConst* constp = VN_CAST(pinp->exprp(), Const); + pins.emplace(varp->name(), constp); + } } } + + auto paramsIt = m_defaultParameterValues.find(modp); + if (paramsIt == m_defaultParameterValues.end()) { // Not cached yet, so check parameters + // Using map with key=string so that we can scan it in deterministic order + DefaultValueMap params; + for (AstNode* stmtp = modp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { + if (AstVar* varp = VN_CAST(stmtp, Var)) { + if (varp->isGParam()) { + AstConst* constp = VN_CAST(varp->valuep(), Const); + // constp can be nullptr if the parameter is not used to instantiate sub + // module. varp->valuep() is not contified yet in the case. + // nullptr means that the parameter is using some default value. + params.emplace(varp->name(), constp); + } + } + } + paramsIt = m_defaultParameterValues.emplace(modp, std::move(params)).first; + } + if (paramsIt->second.empty()) return modp->name(); // modp has no parameter + + string longname = modp->name(); + for (auto&& defaultValue : paramsIt->second) { + auto pinIt = pins.find(defaultValue.first); + AstConst* constp = pinIt == pins.end() ? defaultValue.second : pinIt->second; + // This longname is not valid as verilog symbol, but ok, because it will be hashed + longname += "_" + defaultValue.first + "="; + // constp can be nullptr + if (constp) longname += constp->num().ascii(false); + } + + auto iter = m_longMap.find(longname); + if (iter != m_longMap.end()) return iter->second; // Already calculated + + VHashSha256 hash; + // Calculate hash using longname + // The hash is used as the module suffix to find a module name that is unique in the design + hash.insert(longname); while (true) { // Copy VHashSha256 just in case of hash collision VHashSha256 hashStrGen = hash; @@ -437,7 +505,10 @@ class ParamProcessor final { // Don't use '__' not to be encoded when this module is loaded later by Verilator if (newName.at(newName.size() - 1) != '_') newName += '_'; newName += hashStr.substr(0, i); - if (!moduleExists(newName)) return newName; + if (!moduleExists(newName)) { + m_longMap.emplace(longname, newName); + return newName; + } } // Hash collision. maybe just v3error is practically enough hash.insert(V3Os::trueRandom(64)); @@ -496,15 +567,18 @@ class ParamProcessor final { // DOES clone() so must be finished with module clonep() before here for (AstPin* pinp = paramsp; pinp; pinp = VN_CAST(pinp->nextp(), Pin)) { if (pinp->exprp()) { - if (newmodp->hierBlock()) checkSupportedParam(newmodp, pinp); if (AstVar* modvarp = pinp->modVarp()) { AstNode* newp = pinp->exprp(); // Const or InitArray + AstConst* exprp = VN_CAST(newp, Const); + AstConst* origp = VN_CAST(modvarp->valuep(), Const); + const bool overridden + = !(origp && ParameterizedHierBlocks::areSame(exprp, origp)); // Remove any existing parameter if (modvarp->valuep()) modvarp->valuep()->unlinkFrBack()->deleteTree(); // Set this parameter to value requested by cell UINFO(9, " set param " << modvarp << " = " << newp << endl); modvarp->valuep(newp->cloneTree(false)); - modvarp->overriddenParam(true); + modvarp->overriddenParam(overridden); } else if (AstParamTypeDType* modptp = pinp->modPTypep()) { AstNodeDType* dtypep = VN_CAST(pinp->exprp(), NodeDType); UASSERT_OBJ(dtypep, pinp, "unlinked param dtype"); @@ -680,8 +754,13 @@ public: if (nodep->recursive()) any_overrides = true; if (debug() > 8) nodep->paramsp()->dumpTreeAndNext(cout, "-cellparams: "); - for (AstPin* pinp = nodep->paramsp(); pinp; pinp = VN_CAST(pinp->nextp(), Pin)) { - cellPinCleanup(nodep, pinp, srcModp, longname /*ref*/, any_overrides /*ref*/); + if (srcModp->hierBlock()) { + longname = parameterizedHierBlockName(srcModp, nodep->paramsp()); + any_overrides = longname != srcModp->name(); + } else { + for (AstPin* pinp = nodep->paramsp(); pinp; pinp = VN_CAST(pinp->nextp(), Pin)) { + cellPinCleanup(nodep, pinp, srcModp, longname /*ref*/, any_overrides /*ref*/); + } } IfaceRefRefs ifaceRefRefs; cellInterfaceCleanup(nodep, srcModp, longname /*ref*/, any_overrides /*ref*/, @@ -697,7 +776,7 @@ public: // We need to relink the pins to the new module relinkPinsByName(nodep->pinsp(), paramedModp); } else { - string newname = moduleCalcName(srcModp, nodep->paramsp(), longname); + string newname = srcModp->hierBlock() ? longname : moduleCalcName(srcModp, longname); const ModInfo* modInfop = moduleFindOrClone(srcModp, nodep, nodep->paramsp(), newname, ifaceRefRefs); // Have child use this module instead. diff --git a/test_regress/t/t_hier_block.pl b/test_regress/t/t_hier_block.pl index 5fdce6ee9..fe743cb94 100755 --- a/test_regress/t/t_hier_block.pl +++ b/test_regress/t/t_hier_block.pl @@ -32,7 +32,7 @@ file_grep($Self->{obj_dir} . "/Vsub0/sub0.sv", qr/^\s+\/\/\s+timeprecision\s+(\d file_grep($Self->{obj_dir} . "/Vsub0/sub0.sv", /^module\s+(\S+)\s+/, "sub0"); file_grep($Self->{obj_dir} . "/Vsub1/sub1.sv", /^module\s+(\S+)\s+/, "sub1"); file_grep($Self->{obj_dir} . "/Vsub2/sub2.sv", /^module\s+(\S+)\s+/, "sub2"); -file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 11); +file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 13); file_grep($Self->{run_log_filename}, qr/MACRO:(\S+) is defined/i, "cplusplus"); ok(1); diff --git a/test_regress/t/t_hier_block.v b/test_regress/t/t_hier_block.v index d122a20ad..7c8ee9e15 100644 --- a/test_regress/t/t_hier_block.v +++ b/test_regress/t/t_hier_block.v @@ -291,6 +291,26 @@ module sub5 (input wire clk, input wire [127:0] in[2][3], output logic [7:0] out end end + wire [7:0] val0[2]; + wire [7:0] val1[2]; + wire [7:0] val2[2]; + wire [7:0] val3[2]; + sub6 i_sub0(.out(val0)); + sub6 #(.P0(1)) i_sub1(.out(val1)); // Setting the default value + sub6 #(.P0(1), .P1(2)) i_sub2(.out(val2)); // Setting the default value + sub6 #(.P0(1), .P1(3)) i_sub3(.out(val3)); + + always @(posedge clk) begin + if (val0[0] != 1 || val0[1] != 2) $stop; + if (val1[0] != 1 || val1[1] != 2) $stop; + if (val2[0] != 1 || val2[1] != 2) $stop; + if (val3[0] != 1 || val3[1] != 3) $stop; + end +endmodule + +module sub6 #(parameter P0 = 1, parameter P1 = 2) (output wire [7:0] out[2]); `HIER_BLOCK + assign out[0] = 8'(P0); + assign out[1] = 8'(P1); endmodule module delay #( diff --git a/test_regress/t/t_hier_block_cmake.pl b/test_regress/t/t_hier_block_cmake.pl index 87a83bf39..0517b3a3b 100755 --- a/test_regress/t/t_hier_block_cmake.pl +++ b/test_regress/t/t_hier_block_cmake.pl @@ -33,7 +33,7 @@ if (!$Self->have_cmake) { file_grep($target_dir . 'Vsub0/sub0.sv', /^module\s+(\S+)\s+/, "sub0"); file_grep($target_dir . 'Vsub1/sub1.sv', /^module\s+(\S+)\s+/, "sub1"); file_grep($target_dir . 'Vsub2/sub2.sv', /^module\s+(\S+)\s+/, "sub2"); - file_grep($target_dir . 'Vt_hier_block__stats.txt', qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 11); + file_grep($target_dir . 'Vt_hier_block__stats.txt', qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 13); file_grep($Self->{obj_dir} . '/run.log', qr/MACRO:(\S+) is defined/i, "cplusplus"); } diff --git a/test_regress/t/t_hier_block_sc.pl b/test_regress/t/t_hier_block_sc.pl index c24a54efa..139b280b9 100755 --- a/test_regress/t/t_hier_block_sc.pl +++ b/test_regress/t/t_hier_block_sc.pl @@ -34,7 +34,7 @@ execute( file_grep($Self->{obj_dir} . "/Vsub0/sub0.sv", /^module\s+(\S+)\s+/, "sub0"); file_grep($Self->{obj_dir} . "/Vsub1/sub1.sv", /^module\s+(\S+)\s+/, "sub1"); file_grep($Self->{obj_dir} . "/Vsub2/sub2.sv", /^module\s+(\S+)\s+/, "sub2"); -file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 11); +file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 13); file_grep($Self->{run_log_filename}, qr/MACRO:(\S+) is defined/i, "cplusplus"); ok(1); diff --git a/test_regress/t/t_hier_block_vlt.pl b/test_regress/t/t_hier_block_vlt.pl index 8a7857a11..904182717 100755 --- a/test_regress/t/t_hier_block_vlt.pl +++ b/test_regress/t/t_hier_block_vlt.pl @@ -34,7 +34,7 @@ file_grep($Self->{obj_dir} . "/Vsub0/sub0.sv", qr/^\s+timeprecision\s+(\d+)ps;/m file_grep($Self->{obj_dir} . "/Vsub0/sub0.sv", /^module\s+(\S+)\s+/, "sub0"); file_grep($Self->{obj_dir} . "/Vsub1/sub1.sv", /^module\s+(\S+)\s+/, "sub1"); file_grep($Self->{obj_dir} . "/Vsub2/sub2.sv", /^module\s+(\S+)\s+/, "sub2"); -file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 11); +file_grep($Self->{stats}, qr/HierBlock,\s+Hierarchical blocks\s+(\d+)/i, 13); file_grep($Self->{run_log_filename}, qr/MACRO:(\S+) is defined/i, "cplusplus"); ok(1);