From d7af6436a28a7cb25eca64353da4862e3da1df0f Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 12 Dec 2020 12:56:59 -0500 Subject: [PATCH] Fix genblk naming to match IEEE (#2686). --- Changes | 2 + src/V3LinkDot.cpp | 15 -- src/V3LinkParse.cpp | 50 +++++- src/verilog.y | 5 +- test_regress/t/t_gen_genblk.out | 54 ++++++- test_regress/t/t_gen_genblk.pl | 2 + test_regress/t/t_gen_genblk.v | 205 +++++++++++++++++++++--- test_regress/t/t_gen_genblk_noinl.pl | 2 + test_regress/t/t_gen_intdot2.v | 40 ++--- test_regress/t/t_generate_fatal_bad.out | 2 +- 10 files changed, 307 insertions(+), 70 deletions(-) diff --git a/Changes b/Changes index 43fd4728e..4b4912616 100644 --- a/Changes +++ b/Changes @@ -19,6 +19,8 @@ The contributors that suggested a given feature are shown in []. Thanks! **** Fix showing reference locations for BLKANDNBLK (#2170). [Yuri Victorovich] +**** Fix genblk naming to match IEEE (#2686). [tinshark] + * Verilator 4.106 2020-12-02 diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 792770c29..7aa5f9933 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -718,7 +718,6 @@ class LinkDotFindVisitor final : public AstNVisitor { AstNodeFTask* m_ftaskp = nullptr; // Current function/task bool m_inRecursion = false; // Inside a recursive module int m_paramNum = 0; // Parameter number, for position based connection - int m_blockNum = 0; // Begin block number, 0=none seen bool m_explicitNew = false; // Hit a "new" function int m_modBlockNum = 0; // Begin block number in module, 0=none seen int m_modWithNum = 0; // With block number, 0=none seen @@ -783,7 +782,6 @@ class LinkDotFindVisitor final : public AstNVisitor { VL_RESTORER(m_modSymp); VL_RESTORER(m_curSymp); VL_RESTORER(m_paramNum); - VL_RESTORER(m_blockNum); VL_RESTORER(m_modBlockNum); VL_RESTORER(m_modWithNum); if (doit && nodep->user2()) { @@ -810,7 +808,6 @@ class LinkDotFindVisitor final : public AstNVisitor { } // m_paramNum = 0; - m_blockNum = 0; m_modBlockNum = 0; m_modWithNum = 0; // m_modSymp/m_curSymp for non-packages set by AstCell above this module @@ -845,7 +842,6 @@ class LinkDotFindVisitor final : public AstNVisitor { VL_RESTORER(m_modSymp); VL_RESTORER(m_curSymp); VL_RESTORER(m_paramNum); - VL_RESTORER(m_blockNum); VL_RESTORER(m_modBlockNum); VL_RESTORER(m_modWithNum); { @@ -859,7 +855,6 @@ class LinkDotFindVisitor final : public AstNVisitor { UINFO(9, "New module scope " << m_curSymp << endl); // m_paramNum = 0; - m_blockNum = 0; m_modBlockNum = 0; m_modWithNum = 0; m_explicitNew = false; @@ -937,14 +932,6 @@ class LinkDotFindVisitor final : public AstNVisitor { } virtual void visit(AstNodeBlock* nodep) override { UINFO(5, " " << nodep << endl); - // Rename "genblk"s to include a number - if (m_statep->forPrimary() && !nodep->user4SetOnce()) { - if (nodep->name() == "genblk") { - ++m_blockNum; - nodep->name(nodep->name() + cvtToStr(m_blockNum)); - } - } - // All blocks are numbered in the standard, IE we start with "genblk1" even if only one. if (nodep->name() == "" && nodep->unnamed()) { // Unnamed blocks are only important when they contain var // decls, so search for them. (Otherwise adding all the @@ -962,12 +949,10 @@ class LinkDotFindVisitor final : public AstNVisitor { if (nodep->name() == "") { iterateChildren(nodep); } else { - VL_RESTORER(m_blockNum); VL_RESTORER(m_blockp); VL_RESTORER(m_curSymp); VSymEnt* const oldCurSymp = m_curSymp; { - m_blockNum = 0; m_blockp = nodep; m_curSymp = m_statep->insertBlock(m_curSymp, nodep->name(), nodep, m_classOrPackagep); diff --git a/src/V3LinkParse.cpp b/src/V3LinkParse.cpp index ad2dc5c76..bff977935 100644 --- a/src/V3LinkParse.cpp +++ b/src/V3LinkParse.cpp @@ -57,6 +57,8 @@ private: AstNodeModule* m_modp = nullptr; // Current module AstNodeFTask* m_ftaskp = nullptr; // Current task AstNodeDType* m_dtypep = nullptr; // Current data type + int m_genblkAbove = 0; // Begin block number of if/case/for above + int m_genblkNum = 0; // Begin block number, 0=none seen VLifetime m_lifetime = VLifetime::STATIC; // Propagating lifetime // METHODS @@ -495,12 +497,16 @@ private: V3Config::applyModule(nodep); VL_RESTORER(m_modp); + VL_RESTORER(m_genblkAbove); + VL_RESTORER(m_genblkNum); VL_RESTORER(m_lifetime); { // Module: Create sim table for entire module and iterate cleanFileline(nodep); // m_modp = nodep; + m_genblkAbove = 0; + m_genblkNum = 0; m_valueModp = nodep; m_lifetime = nodep->lifetime(); if (m_lifetime.isNone()) { @@ -538,13 +544,45 @@ private: || VN_IS(nodep->stmtsp(), GenCase)) // Has an if/case && !nodep->stmtsp()->nextp()); // Has only one item // It's not FOR(BEGIN(...)) but we earlier changed it to BEGIN(FOR(...)) - if (nodep->genforp() && nodep->name() == "") { - nodep->name("genblk"); - } else if (nodep->generate() && nodep->name() == "" - && (VN_IS(backp, CaseItem) || VN_IS(backp, GenIf)) && !nestedIf) { - nodep->name("genblk"); + if (nodep->genforp()) { + ++m_genblkNum; + if (nodep->name() == "") nodep->name("genblk" + cvtToStr(m_genblkNum)); + } + if (nodep->generate() && nodep->name() == "" + && (VN_IS(backp, CaseItem) || VN_IS(backp, GenIf)) && !nestedIf) { + nodep->name("genblk" + cvtToStr(m_genblkAbove)); + } + if (nodep->name() != "") { + VL_RESTORER(m_genblkAbove); + VL_RESTORER(m_genblkNum); + m_genblkAbove = 0; + m_genblkNum = 0; + iterateChildren(nodep); + } else { + iterateChildren(nodep); + } + } + virtual void visit(AstGenCase* nodep) override { + ++m_genblkNum; + cleanFileline(nodep); + { + VL_RESTORER(m_genblkAbove); + VL_RESTORER(m_genblkNum); + m_genblkAbove = m_genblkNum; + m_genblkNum = 0; + iterateChildren(nodep); + } + } + virtual void visit(AstGenIf* nodep) override { + ++m_genblkNum; + cleanFileline(nodep); + { + VL_RESTORER(m_genblkAbove); + VL_RESTORER(m_genblkNum); + m_genblkAbove = m_genblkNum; + m_genblkNum = 0; + iterateChildren(nodep); } - iterateChildren(nodep); } virtual void visit(AstCase* nodep) override { V3Config::applyCase(nodep); diff --git a/src/verilog.y b/src/verilog.y index c4d507725..9ee9888ea 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -2465,7 +2465,8 @@ loop_generate_construct: // ==IEEE: loop_generate_construct { // Convert BEGIN(...) to BEGIN(GENFOR(...)), as we need the BEGIN to hide the local genvar AstBegin* lowerBegp = VN_CAST($9, Begin); UASSERT_OBJ(!($9 && !lowerBegp), $9, "Child of GENFOR should have been begin"); - if (!lowerBegp) lowerBegp = new AstBegin($1, "genblk", nullptr, true, true); // Empty body + + if (!lowerBegp) lowerBegp = new AstBegin($1, "", nullptr, true, false); // Empty body AstNode* lowerNoBegp = lowerBegp->stmtsp(); if (lowerNoBegp) lowerNoBegp->unlinkFrBackWithNext(); // @@ -2479,7 +2480,7 @@ loop_generate_construct: // ==IEEE: loop_generate_construct } // Statements are under 'genforp' as cells under this // for loop won't get an extra layer of hierarchy tacked on - blkp->addGenforp(new AstGenFor($1,initp,$5,$7,lowerNoBegp)); + blkp->addGenforp(new AstGenFor($1, initp, $5, $7, lowerNoBegp)); $$ = blkp; VL_DO_DANGLING(lowerBegp->deleteTree(), lowerBegp); } diff --git a/test_regress/t/t_gen_genblk.out b/test_regress/t/t_gen_genblk.out index 4eb73f9a4..9a6cb52a4 100644 --- a/test_regress/t/t_gen_genblk.out +++ b/test_regress/t/t_gen_genblk.out @@ -1,6 +1,50 @@ -015: exp=top.t.show0 got=top.t.show0 -019: exp=top.t.genblk1.show1 got=top.t.genblk1.show1 -023: exp=top.t.genblk2.show2 got=top.t.genblk2.show2 -028: exp=top.t.genblk3.genblk1.show3 got=top.t.genblk3.genblk1.show3 -034: exp=top.t.x1.x3.show4 got=top.t.x1.x3.show4 +021: got=top.t.direct_ignored.show1 +023: got=top.t.direct_ignored.genblk1.show2 exp=1 gennum=1 + +030: got=top.t.empty_DISAGREE.genblk1.show2 exp=0 gennum=1 + +037: got=top.t.empty_named_DISAGREE.genblk1.show2 exp=0 gennum=1 + +043: got=top.t.unnamed_counts.show1 +046: got=top.t.unnamed_counts.genblk1.show2 exp=0 gennum=1 + +052: got=top.t.named_counts.named.show1 +055: got=top.t.named_counts.genblk1.show2 exp=0 gennum=1 + +061: got=top.t.if_direct_counts.genblk1.show1 +063: got=top.t.if_direct_counts.genblk2.show2 exp=2 gennum=2 + +069: got=top.t.if_begin_counts.genblk1.show1 +071: got=top.t.if_begin_counts.genblk2.show2 exp=2 gennum=2 + +076: got=top.t.if_named_counts.named.show1 +078: got=top.t.if_named_counts.named.subnamed.show1s +082: got=top.t.if_named_counts.genblk2.show2 exp=2 gennum=2 + +089: got=top.t.begin_if_counts.genblk1.show1 +092: got=top.t.begin_if_counts.genblk2.show2 exp=2 gennum=2 + +099: got=top.t.for_empty_counts.genblk2.show2 exp=0 gennum=2 + +104: got=top.t.for_direct_counts.genblk1[0].show1 +106: got=top.t.for_direct_counts.genblk2.show2 exp=2 gennum=2 + +111: got=top.t.for_named_counts.fornamed[0].show1 +114: got=top.t.for_named_counts.genblk2.show2 exp=2 gennum=2 + +119: got=top.t.for_begin_counts.genblk1[0].show1 +122: got=top.t.for_begin_counts.genblk2.show2 exp=2 gennum=2 + +132: got=top.t.if_if.genblk1.genblk1.show1 +136: got=top.t.if_if.genblk2.show2 exp=2 gennum=2 + +142: got=top.t.case_direct.genblk1.show1 +146: got=top.t.case_direct.genblk2.show2 exp=2 gennum=2 + +152: got=top.t.case_begin_counts.genblk1.show1 +156: got=top.t.case_begin_counts.genblk2.show2 exp=2 gennum=2 + +162: got=top.t.case_named_counts.subnamed.show1 +166: got=top.t.case_named_counts.genblk2.show2 exp=2 gennum=2 + *-* All Finished *-* diff --git a/test_regress/t/t_gen_genblk.pl b/test_regress/t/t_gen_genblk.pl index 56cc415df..e86489964 100755 --- a/test_regress/t/t_gen_genblk.pl +++ b/test_regress/t/t_gen_genblk.pl @@ -10,6 +10,8 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(simulator => 1); +$Self->{sim_time} = 11000; + compile( ); diff --git a/test_regress/t/t_gen_genblk.v b/test_regress/t/t_gen_genblk.v index ee2dd3ea4..0a9b85582 100644 --- a/test_regress/t/t_gen_genblk.v +++ b/test_regress/t/t_gen_genblk.v @@ -3,6 +3,11 @@ // any use, without warranty, 2020 by Wilson Snyder. // SPDX-License-Identifier: CC0-1.0 +`define CONCAT(a,b) a``b +`define SHOW_LINED `CONCAT(show, `__LINE__) + +bit fails; + module t (/*AUTOARG*/ // Inputs clk, reset_l @@ -12,45 +17,209 @@ module t (/*AUTOARG*/ input reset_l; generate - show #(`__LINE__, "top.t.show0") show0(); + begin : direct_ignored + show #(`__LINE__) show1(); - if (0) ; - else if (0) ; - else if (1) show #(`__LINE__, "top.t.genblk1.show1") show1(); + if (1) begin check #(`__LINE__, 1) show2(); end + end - if (0) begin end - else if (0) begin end - else if (1) begin show #(`__LINE__, "top.t.genblk2.show2") show2(); end + begin : empty_DISAGREE + // DISAGREEMENT: if empty unnamed begin/end counts + begin end - if (0) ; - else begin + if (1) begin check #(`__LINE__, 0) show2(); end + end + + begin : empty_named_DISAGREE + // DISAGREEMENT: if empty named begin/end counts + begin : empty_inside_named end + + if (1) begin check #(`__LINE__, 0) show2(); end + end + + begin : unnamed_counts + // DISAGREEMENT: if unnamed begin/end counts + begin + show #(`__LINE__) show1(); + end + + if (1) begin check #(`__LINE__, 0) show2(); end + end + + begin : named_counts + // DISAGREEMENT: if named begin/end counts + begin : named + show #(`__LINE__) show1(); + end + + if (1) begin check #(`__LINE__, 0) show2(); end + end + + begin : if_direct_counts + if (0) ; + else if (0) ; + else if (1) show #(`__LINE__) show1(); + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : if_begin_counts if (0) begin end - else if (1) begin show #(`__LINE__, "top.t.genblk3.genblk1.show3") show3(); end + else if (0) begin show #(`__LINE__) show1_NOT(); end + else if (1) begin show #(`__LINE__) show1(); end + + if (1) begin check #(`__LINE__, 2) show2(); end end - if (0) ; - else begin : x1 - if (0) begin : x2 end - else if (1) begin : x3 show #(`__LINE__, "top.t.x1.x3.show4") show4(); end + begin : if_named_counts + if (1) begin : named + show #(`__LINE__) show1(); + if (1) begin : subnamed + show #(`__LINE__) show1s(); + end + end + + if (1) begin check #(`__LINE__, 2) show2(); end end + + begin : begin_if_counts + begin + if (0) begin end + else if (0) begin show #(`__LINE__) show1_NOT(); end + else if (1) begin show #(`__LINE__) show1(); end + end + // DISAGREEMENT: this could be genblk01 + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : for_empty_counts + // DISAGREEMENT: if empty genfor counts + for (genvar g = 0; g < 1; ++g) ; + + if (1) begin check #(`__LINE__, 0) show2(); end + end + + begin : for_direct_counts + for (genvar g = 0; g < 1; ++g) + show #(`__LINE__) show1(); + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : for_named_counts + for (genvar g = 0; g < 1; ++g) begin : fornamed + show #(`__LINE__) show1(); + end + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : for_begin_counts + for (genvar g = 0; g < 1; ++g) begin + show #(`__LINE__) show1(); + end + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : if_if + if (0) ; + else if (0) begin : namedb + end + else begin + if (0) begin end + else if (1) begin + show #(`__LINE__) show1(); + end + end + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : case_direct + case (1) + 0 : show #(`__LINE__) show1a_NOT(); + 1 : show #(`__LINE__) show1(); + 2 : show #(`__LINE__) show1c_NOT(); + endcase + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : case_begin_counts + case (1) + 0 : begin show #(`__LINE__) show1a_NOT(); end + 1 : begin show #(`__LINE__) show1(); end + 2 : begin show #(`__LINE__) show1c_NOT(); end + endcase + + if (1) begin check #(`__LINE__, 2) show2(); end + end + + begin : case_named_counts + case (1) + 0 : begin : subnamed show #(`__LINE__) show1a_NOT(); end + 1 : begin : subnamed show #(`__LINE__) show1(); end + 2 : begin : subnamed show #(`__LINE__) show1c_NOT(); end + endcase + + if (1) begin check #(`__LINE__, 2) show2(); end + end + endgenerate int cyc; always @ (posedge clk) begin cyc <= cyc + 1; - if (cyc == 99) begin - $write("*-* All Finished *-*\n"); + if (cyc == 999) begin + if (fails) $stop; + else $write("*-* All Finished *-*\n"); $finish; end end endmodule -module show #(parameter LINE=0, parameter string EXPT) (); +module show #(parameter LINE=0) (); + // Each instance compares on unique cycle based on line number + // so we get deterministic ordering (versus using an initial) always @ (posedge t.clk) begin if (t.cyc == LINE) begin - $display("%03d: exp=%s got=%m", LINE, EXPT); + $display("%03d: got=%m", LINE); + end + end +endmodule + +module check #(parameter LINE=0, parameter EXP=0) (); + string mod; + int gennum; + int pos; + + always @ (posedge t.clk) begin + if (t.cyc == LINE) begin + mod = $sformatf("%m"); + + gennum = 0; + for (int pos = 0; pos < mod.len(); ++pos) begin + if (mod.substr(pos, pos+5) == "genblk") begin + pos += 6; + // verilator lint_off WIDTH + gennum = mod[pos] - "0"; + // verilator lint_on WIDTH + break; + end + end + + $write("%03d: got=%s exp=%0d gennum=%0d ", LINE, mod, EXP, gennum); + if (EXP == 0) $display(" "); + else if (gennum != EXP) begin + $display (" %%Error"); + fails = 1; + end + else $display; + + $display; end end endmodule diff --git a/test_regress/t/t_gen_genblk_noinl.pl b/test_regress/t/t_gen_genblk_noinl.pl index 08623e342..86adb4a8e 100755 --- a/test_regress/t/t_gen_genblk_noinl.pl +++ b/test_regress/t/t_gen_genblk_noinl.pl @@ -12,6 +12,8 @@ top_filename("t_gen_genblk.v"); scenarios(simulator => 1); +$Self->{sim_time} = 11000; + compile( v_flags2 => ["-Oi"], ); diff --git a/test_regress/t/t_gen_intdot2.v b/test_regress/t/t_gen_intdot2.v index d405a9d47..a2f3886ca 100644 --- a/test_regress/t/t_gen_intdot2.v +++ b/test_regress/t/t_gen_intdot2.v @@ -64,11 +64,8 @@ module Genit ( else One ifcell1(); // genblk1.ifcell1 endgenerate - // On compliant simulators "Implicit name" not allowed here; IE we can't use "genblk1" etc -`ifdef verilator + // DISAGREEMENT on this naming always @ (posedge clk) if (genblk1.ifcell1.one !== 1'b1) $stop; -//`else // NOT SUPPORTED accoring to spec - generic block references -`endif generate begin : namedif2 @@ -76,10 +73,8 @@ module Genit ( One ifcell2(); // namedif2.genblk1.ifcell2 end endgenerate -`ifdef verilator + // DISAGREEMENT on this naming always @ (posedge clk) if (namedif2.genblk1.ifcell2.one !== 1'b1) $stop; -//`else // NOT SUPPORTED accoring to spec - generic block references -`endif generate if (1'b1) @@ -91,15 +86,15 @@ module Genit ( // CASE generate - case (1'b1) - 1'b1 : - One casecell10(); // genblk3.casecell10 - endcase + begin : casecheck + case (1'b1) + 1'b1 : + One casecell10(); // genblk4.casecell10 + endcase + end endgenerate -`ifdef verilator - always @ (posedge clk) if (genblk3.casecell10.one !== 1'b1) $stop; -//`else // NOT SUPPORTED accoring to spec - generic block references -`endif + // DISAGREEMENT on this naming + always @ (posedge clk) if (casecheck.genblk1.casecell10.one !== 1'b1) $stop; generate case (1'b1) @@ -113,16 +108,15 @@ module Genit ( genvar i; genvar j; - // IF generate - for (i = 0; i < 2; i = i + 1) - One cellfor20 (); // genblk4[0..1].cellfor20 + begin : genfor + for (i = 0; i < 2; i = i + 1) + One cellfor20 (); // genfor.genblk1[0..1].cellfor20 + end endgenerate -`ifdef verilator - always @ (posedge clk) if (genblk4[0].cellfor20.one !== 1'b1) $stop; - always @ (posedge clk) if (genblk4[1].cellfor20.one !== 1'b1) $stop; -//`else // NOT SUPPORTED accoring to spec - generic block references -`endif + // DISAGREEMENT on this naming + always @ (posedge clk) if (genfor.genblk1[0].cellfor20.one !== 1'b1) $stop; + always @ (posedge clk) if (genfor.genblk1[1].cellfor20.one !== 1'b1) $stop; // COMBO generate diff --git a/test_regress/t/t_generate_fatal_bad.out b/test_regress/t/t_generate_fatal_bad.out index 1e3d4e2b1..0a0bc2a9b 100644 --- a/test_regress/t/t_generate_fatal_bad.out +++ b/test_regress/t/t_generate_fatal_bad.out @@ -50,7 +50,7 @@ 13 | localparam integer BAZ = get_baz(BAR); | ^~~~~~~ %Error: t/t_generate_fatal_bad.v:13:29: Expecting expression to be constant, but can't determine constant for FUNCREF 'get_baz' - : ... In instance t.genblk1.foo_inst4 + : ... In instance t.genblk4.foo_inst4 t/t_generate_fatal_bad.v:9:4: ... Location of non-constant STOP: $stop executed during function constification; maybe indicates assertion firing t/t_generate_fatal_bad.v:13:29: ... Called from get_baz() with parameters: bar = ?32?h7