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

@@ -71,7 +71,9 @@ class ParameterizedHierBlocks final {
     typedef HierBlockOptsByOrigName::const_iterator HierMapIt;
     typedef std::map<const string, AstNodeModule*> HierBlockModMap;
     typedef std::map<const string, std::unique_ptr<AstConst>> ParamConstMap;
+    typedef std::map<const string, AstVar*> GParamsMap;  // key:parameter name value:parameter
     typedef std::map<const V3HierarchicalBlockOption*, ParamConstMap> HierParamsMap;
+    typedef std::map<const string, GParamsMap> 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<HierMapIt, HierMapIt> 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<string, AstConst*> DefaultValueMap;
+    // Default parameter values of hierarchical blocks
+    std::map<AstNodeModule*, DefaultValueMap> 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 = srcModp->name();
-                    newname = parametrizedHierBlockName(srcModp, paramsp);
+                // We use all upper case above, so lower here can't conflict
-                } else {
+                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 {
+    string parameterizedHierBlockName(AstNodeModule* modp, AstPin* paramPinsp) {
-        VHashSha256 hash;
+        // Create a unique name in the following steps
-        // Calculate hash using module name, parameter name, and parameter value
+        //  - Make a long name that includes all parameters, that appear
-        // The hash is used as the module suffix to find a module name that is unique in the design
+        //    in the alphabetical order.
-        hash.insert(modp->name());
+        //  - Hash the long name to get valid Verilog symbol
+        UASSERT_OBJ(modp->hierBlock(), modp, "should be used for hierarchical block");
+
+        std::map<string, AstConst*> pins;
         for (AstPin* pinp = paramPinsp; pinp; pinp = VN_CAST(pinp->nextp(), Pin)) {
-            if (AstVar* varp = pinp->modVarp()) hash.insert(varp->name());
+            checkSupportedParam(modp, pinp);
-            if (AstConst* constp = VN_CAST(pinp->exprp(), Const)) {
+            if (AstVar* varp = pinp->modVarp()) {
-                hash.insert(constp->num().ascii(false));
+                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)) {
+        if (srcModp->hierBlock()) {
-            cellPinCleanup(nodep, pinp, srcModp, longname /*ref*/, any_overrides /*ref*/);
+            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.

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);

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 #(

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");
 }
 

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);

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);