From 60eab3eb8cb5862155137d25b0fa513bcd864d95 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Sun, 24 Jul 2022 19:54:37 +0900 Subject: [PATCH 01/11] Fix wrong result of bit op tree optimization #3509 (#3516) * Tests: Add a test to reproduce #3509 * Tests: Compile without tautological-compare check because bit op tree optimization is disabled in the test. * Internals: Dedup code. No functional change is intended. * Fix #3509. "2'b10 == (2'b11 & {1'b0, val[0]})" and "2'b10 != (2'b11 & {1'b0, val[0]})" were wrongly optimized to "!val[0]" and "val[0]" respectively. Now properly optimize them to 1'b0 and 1'b1. * Commentary * Commentary: Update Changes --- Changes | 1 + src/V3Const.cpp | 45 ++++++++++++++++--------- test_regress/t/t_const_no_opt.pl | 9 ++++- test_regress/t/t_const_opt.v | 56 +++++++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 18 deletions(-) diff --git a/Changes b/Changes index 910eceb2f..9b7dd96e8 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,7 @@ Verilator 4.225 devel * Fix incorrect bit op tree optimization (#3470). [algrobman] * Fix empty string arguments to display (#3484). [Grulfen] * Fix table misoptimizing away display (#3488). [Stefan Post] +* Fix wrong bit op tree optimization (#3509). [Nathan Graybeal] Verilator 4.224 2022-06-19 diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 700a99e38..6f543fdba 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -566,9 +566,35 @@ class ConstBitOpTreeVisitor final : public VNVisitor { const AstConst* const constp = VN_CAST(lhsp, Const); CONST_BITOP_RETURN_IF(!constp, nodep->lhsp()); - const bool maskFlip = isOrTree(); const V3Number& compNum = constp->num(); + auto setPolarities = [this, &compNum](const LeafInfo& ref, const V3Number* maskp) { + const bool maskFlip = isOrTree(); + int constantWidth = compNum.width(); + if (maskp) constantWidth = std::max(constantWidth, maskp->width()); + const int maxBitIdx = std::max(ref.lsb() + constantWidth, ref.msb() + 1); + // Mark all bits checked by this comparison + for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { + const int maskIdx = bitIdx - ref.lsb(); + const bool mask0 = maskp && maskp->bitIs0(maskIdx); + const bool outOfRange = bitIdx > ref.msb(); + if (mask0 || outOfRange) { // RHS is 0 + if (compNum.bitIs1(maskIdx)) { + // LHS is 1 + // And tree: 1 == 0 => always false, set v && !v + // Or tree : 1 != 0 => always true, set v || !v + m_bitPolarities.emplace_back(ref, true, 0); + m_bitPolarities.emplace_back(ref, false, 0); + break; + } else { // This bitIdx is irrelevant + continue; + } + } + const bool polarity = compNum.bitIs1(maskIdx) != maskFlip; + m_bitPolarities.emplace_back(ref, polarity, bitIdx); + } + }; + if (const AstAnd* const andp = VN_CAST(nodep->rhsp(), And)) { // comp == (mask & v) const LeafInfo& mask = findLeaf(andp->lhsp(), true); CONST_BITOP_RETURN_IF(!mask.constp() || mask.lsb() != 0, andp->lhsp()); @@ -583,14 +609,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { incrOps(nodep, __LINE__); incrOps(andp, __LINE__); - // Mark all bits checked by this comparison - const int maxBitIdx = std::min(ref.lsb() + maskNum.width(), ref.msb() + 1); - for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { - const int maskIdx = bitIdx - ref.lsb(); - if (maskNum.bitIs0(maskIdx)) continue; - const bool polarity = compNum.bitIs1(maskIdx) != maskFlip; - m_bitPolarities.emplace_back(ref, polarity, bitIdx); - } + setPolarities(ref, &maskNum); } else { // comp == v const LeafInfo& ref = findLeaf(nodep->rhsp(), false); CONST_BITOP_RETURN_IF(!ref.refp(), nodep->rhsp()); @@ -599,13 +618,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor { incrOps(nodep, __LINE__); - // Mark all bits checked by this comparison - const int maxBitIdx = std::min(ref.lsb() + compNum.width(), ref.msb() + 1); - for (int bitIdx = ref.lsb(); bitIdx < maxBitIdx; ++bitIdx) { - const int maskIdx = bitIdx - ref.lsb(); - const bool polarity = compNum.bitIs1(maskIdx) != maskFlip; - m_bitPolarities.emplace_back(ref, polarity, bitIdx); - } + setPolarities(ref, nullptr); } } else { CONST_BITOP_SET_FAILED("Mixture of different ops cannot be optimized", nodep); diff --git a/test_regress/t/t_const_no_opt.pl b/test_regress/t/t_const_no_opt.pl index 79bc15076..db1a41047 100755 --- a/test_regress/t/t_const_no_opt.pl +++ b/test_regress/t/t_const_no_opt.pl @@ -13,7 +13,14 @@ top_filename("t/t_const_opt.v"); # Run the same design as t_const_opt.pl without bitopt tree optimization to make sure that the result is same. compile( - verilator_flags2 => ["-Wno-UNOPTTHREADS", "--stats", "-fno-const-bit-op-tree", "$Self->{t_dir}/t_const_opt.cpp"], + verilator_flags2 => [ + "-Wno-UNOPTTHREADS", + "--stats", + "-fno-const-bit-op-tree", + "$Self->{t_dir}/t_const_opt.cpp", + "-CFLAGS", + "-Wno-tautological-compare" + ], ); execute( diff --git a/test_regress/t/t_const_opt.v b/test_regress/t/t_const_opt.v index e24d28bf8..559477475 100644 --- a/test_regress/t/t_const_opt.v +++ b/test_regress/t/t_const_opt.v @@ -87,10 +87,11 @@ module Test(/*AUTOARG*/ logic bug3197_out; logic bug3445_out; logic bug3470_out; + logic bug3509_out; output logic o; - logic [8:0] tmp; + logic [9:0] tmp; assign o = ^tmp; always_ff @(posedge clk) begin @@ -115,12 +116,14 @@ module Test(/*AUTOARG*/ tmp[6] <= bug3197_out; tmp[7] <= bug3445_out; tmp[8] <= bug3470_out; + tmp[9] <= bug3509_out; end bug3182 i_bug3182(.in(d[4:0]), .out(bug3182_out)); bug3197 i_bug3197(.clk(clk), .in(d), .out(bug3197_out)); bug3445 i_bug3445(.clk(clk), .in(d), .out(bug3445_out)); bug3470 i_bug3470(.clk(clk), .in(d), .out(bug3470_out)); + bug3509 i_bug3509(.clk(clk), .in(d), .out(bug3509_out)); endmodule @@ -235,3 +238,54 @@ module bug3470(input wire clk, input wire [31:0] in, output wire out); assign out = tmp; endmodule + +// Bug3509 +// Only bit range of "var" was considered in +// "comp == (mask & var)" +// and +// "comp != (mask & var)" +// +// It caused wrong result if "comp" has wider bit width because +// upper bit of "comp" was ignored. +// +// If "comp" has '1' in upper bit range than "var", +// the result is constant after optimization. +module bug3509(input wire clk, input wire [31:0] in, output reg out); + reg [2:0] r0; + always_ff @(posedge clk) + r0 <= in[2:0]; + + wire [3:0] w1_0 = {1'b0, in[2:0]}; + wire [3:0] w1_1 = {1'b0, r0}; + + wire tmp[4]; + + // tmp[0:1] is always 0 because w1[3] == 1'b0 + // tmp[2:3] is always 1 because w1[3] == 1'b0 + assign tmp[0] = w1_0[3:2] == 2'h2 && w1_0[1:0] != 2'd3; + assign tmp[1] = w1_1[3:2] == 2'h2 && w1_1[1:0] != 2'd3; + assign tmp[2] = w1_0[3:2] != 2'h2 || w1_0[1:0] == 2'd3; + assign tmp[3] = w1_1[3:2] != 2'h2 || w1_1[1:0] == 2'd3; + always_ff @(posedge clk) begin + out <= tmp[0] | tmp[1] | !tmp[2] | !tmp[3]; + end + + always @(posedge clk) begin + if(tmp[0]) begin + $display("tmp[0] != 0"); + $stop; + end + if(tmp[1]) begin + $display("tmp[1] != 0"); + $stop; + end + if(!tmp[2]) begin + $display("tmp[2] != 1"); + $stop; + end + if(!tmp[3]) begin + $display("tmp[3] != 1"); + $stop; + end + end +endmodule From 89924bda51d2a170c23ab18e1c8efa7bab57f162 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 12 Nov 2021 16:46:58 +0000 Subject: [PATCH 02/11] Always type '$clog2' as signed 32 --- src/V3AstNodes.h | 4 +++- src/V3Width.cpp | 5 +---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h index 8b258db0a..f86f8e350 100644 --- a/src/V3AstNodes.h +++ b/src/V3AstNodes.h @@ -6139,7 +6139,9 @@ public: class AstCLog2 final : public AstNodeUniop { public: AstCLog2(FileLine* fl, AstNode* lhsp) - : ASTGEN_SUPER_CLog2(fl, lhsp) {} + : ASTGEN_SUPER_CLog2(fl, lhsp) { + dtypeSetSigned32(); + } ASTNODE_NODE_FUNCS(CLog2) virtual void numberOperate(V3Number& out, const V3Number& lhs) override { out.opCLog2(lhs); } virtual string emitVerilog() override { return "%f$clog2(%l)"; } diff --git a/src/V3Width.cpp b/src/V3Width.cpp index c98e856aa..446f20ed8 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -1248,10 +1248,7 @@ private: } } virtual void visit(AstCLog2* nodep) override { - if (m_vup->prelim()) { - iterateCheckSizedSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); - nodep->dtypeSetSigned32(); - } + if (m_vup->prelim()) { iterateCheckSizedSelf(nodep, "LHS", nodep->lhsp(), SELF, BOTH); } } virtual void visit(AstPow* nodep) override { // Pow is special, output sign only depends on LHS sign, but From 290c2e03884284ccb984da1d50ffecf1dd08459b Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Mon, 25 Jul 2022 12:51:02 +0100 Subject: [PATCH 03/11] Mark FileLine::v3errorEndFatal as noreturn --- src/V3FileLine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/V3FileLine.h b/src/V3FileLine.h index 1ff0c06a4..328b682cd 100644 --- a/src/V3FileLine.h +++ b/src/V3FileLine.h @@ -247,7 +247,7 @@ public: // OPERATORS void v3errorEnd(std::ostringstream& str, const string& extra = ""); - void v3errorEndFatal(std::ostringstream& str); + void v3errorEndFatal(std::ostringstream& str) VL_ATTR_NORETURN; /// When building an error, prefix for printing continuation lines /// e.g. information referring to the same FileLine as before string warnMore() const; From ac4ec879426905b1dcbd75dd0ccdbab26514bb0d Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Mon, 25 Jul 2022 12:59:26 +0100 Subject: [PATCH 04/11] Respect clang's default -fbracket-depth by default Set default value of --comp-limit-parens to 240, to respect default maximum nesting of parentheses in clang (which is controlled by -fbracket-depth and defaults to 256). For code generation consistency, also use the same default with gcc. --- src/V3Options.cpp | 4 ++-- src/V3Options.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/V3Options.cpp b/src/V3Options.cpp index e163278f8..29c4a4ca3 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1010,11 +1010,11 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, char if (!strcmp(valp, "clang")) { m_compLimitBlocks = 80; // limit unknown m_compLimitMembers = 64; // soft limit, has slowdown bug as of clang++ 3.8 - m_compLimitParens = 80; // limit unknown + m_compLimitParens = 240; // controlled by -fbracket-depth, which defaults to 256 } else if (!strcmp(valp, "gcc")) { m_compLimitBlocks = 0; // Bug free m_compLimitMembers = 64; // soft limit, has slowdown bug as of g++ 7.1 - m_compLimitParens = 0; // Bug free + m_compLimitParens = 240; // Unlimited, but generate same code as for clang } else if (!strcmp(valp, "msvc")) { m_compLimitBlocks = 80; // 128, but allow some room m_compLimitMembers = 0; // probably ok, and AFAIK doesn't support anon structs diff --git a/src/V3Options.h b/src/V3Options.h index 78fa1b8bd..4db4e9d05 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -316,7 +316,7 @@ private: int m_compLimitBlocks = 0; // compiler selection; number of nested blocks int m_compLimitMembers = 64; // compiler selection; number of members in struct before make anon array - int m_compLimitParens = 0; // compiler selection; number of nested parens + int m_compLimitParens = 240; // compiler selection; number of nested parens string m_bin; // main switch: --bin {binary} string m_exeName; // main switch: -o {name} From eeef5ab4de04810c3ad9d2f96c3ceb037957cb70 Mon Sep 17 00:00:00 2001 From: Gustav Svensk Date: Mon, 25 Jul 2022 17:36:34 +0200 Subject: [PATCH 05/11] Fix sformat string incorrectly cleared (#3515) (#3519). --- docs/CONTRIBUTORS | 1 + include/verilated.cpp | 5 +++-- test_regress/t/t_sys_sformat.v | 6 ++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/CONTRIBUTORS b/docs/CONTRIBUTORS index a80d950b9..77e232945 100644 --- a/docs/CONTRIBUTORS +++ b/docs/CONTRIBUTORS @@ -33,6 +33,7 @@ Gianfranco Costamagna Glen Gibb Graham Rushton Guokai Chen +Gustav Svensk Harald Heckmann Howard Su Huang Rui diff --git a/include/verilated.cpp b/include/verilated.cpp index 7b9200363..49ef170dc 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -1459,11 +1459,12 @@ void VL_SFORMAT_X(int obits, void* destp, const char* formatp, ...) VL_MT_SAFE { void VL_SFORMAT_X(int obits_ignored, std::string& output, const char* formatp, ...) VL_MT_SAFE { if (obits_ignored) {} - output = ""; + std::string temp_output; va_list ap; va_start(ap, formatp); - _vl_vsformat(output, formatp, ap); + _vl_vsformat(temp_output, formatp, ap); va_end(ap); + output = temp_output; } std::string VL_SFORMATF_NX(const char* formatp, ...) VL_MT_SAFE { diff --git a/test_regress/t/t_sys_sformat.v b/test_regress/t/t_sys_sformat.v index 2e3443e5b..bf8bc8a5b 100644 --- a/test_regress/t/t_sys_sformat.v +++ b/test_regress/t/t_sys_sformat.v @@ -83,6 +83,12 @@ module t; $swriteo(str2, 4'd12); if (str2 != "14") $stop; + str3 = "foo"; + $sformat(str3, "%s", str3); // $sformat twice so verilator does not + $sformat(str3, "%s", str3); // optimize the call to $sformat(str3, "%s", "foo") +`ifdef TEST_VERBOSE $display("str3=%0s", str3); `endif + if (str3 != "foo") $stop; + $write("*-* All Finished *-*\n"); $finish; end From 7b431b37c7cb92c9138b53104e210c3d7ccec7ff Mon Sep 17 00:00:00 2001 From: Mostafa Gamal Date: Mon, 25 Jul 2022 23:46:22 +0200 Subject: [PATCH 06/11] Fix struct pattern assignment (#2328) (#3517). --- docs/CONTRIBUTORS | 1 + src/V3Width.cpp | 94 +++++++----- src/verilog.y | 1 + test_regress/t/t_array_list_bad.out | 2 +- .../t/t_structu_dataType_assignment.pl | 21 +++ .../t/t_structu_dataType_assignment.v | 137 ++++++++++++++++++ .../t/t_structu_dataType_assignment_bad.out | 5 + .../t/t_structu_dataType_assignment_bad.pl | 21 +++ .../t/t_structu_dataType_assignment_bad.v | 21 +++ 9 files changed, 269 insertions(+), 34 deletions(-) create mode 100644 test_regress/t/t_structu_dataType_assignment.pl create mode 100644 test_regress/t/t_structu_dataType_assignment.v create mode 100644 test_regress/t/t_structu_dataType_assignment_bad.out create mode 100755 test_regress/t/t_structu_dataType_assignment_bad.pl create mode 100644 test_regress/t/t_structu_dataType_assignment_bad.v diff --git a/docs/CONTRIBUTORS b/docs/CONTRIBUTORS index 77e232945..0b003034b 100644 --- a/docs/CONTRIBUTORS +++ b/docs/CONTRIBUTORS @@ -82,6 +82,7 @@ Michaël Lefebvre Mike Popoloski Miodrag Milanović Morten Borup Petersen +Mostafa Gamal Nandu Raj Nathan Kohagen Nathan Myers diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 446f20ed8..9ce0b5b4c 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -3559,50 +3559,67 @@ private: // which member each AstPatMember corresponds to before we can // determine the dtypep for that PatMember's value, and then // width the initial value appropriately. - using PatMap = std::map; + using PatMap = std::map; // Store member: value + using DTypeMap = std::map; // Store data_type: default_value PatMap patmap; + DTypeMap dtypemap; { const AstMemberDType* memp = vdtypep->membersp(); AstPatMember* patp = VN_CAST(nodep->itemsp(), PatMember); - for (; memp || patp;) { + while (patp) { do { - if (patp) { - if (patp->keyp()) { - if (const AstText* textp = VN_CAST(patp->keyp(), Text)) { - memp = vdtypep->findMember(textp->text()); - if (!memp) { - patp->keyp()->v3error("Assignment pattern key '" - << textp->text() - << "' not found as member"); - break; - } + if (patp->keyp()) { + // '{member:value} or '{data_type: default_value} + if (const AstText* textp = VN_CAST(patp->keyp(), Text)) { + // member: value + memp = vdtypep->findMember(textp->text()); + if (!memp) { + patp->keyp()->v3error("Assignment pattern key '" + << textp->text() + << "' not found as member"); + break; } else { - patp->keyp()->v3error( - "Assignment pattern key not supported/understood: " - << patp->keyp()->prettyTypeName()); + const std::pair ret = patmap.emplace(memp, patp); + if (!ret.second) { + patp->v3error("Assignment pattern contains duplicate entry: " + << VN_AS(patp->keyp(), Text)->text()); + } + memp = VN_AS(memp->nextp(), MemberDType); } } + else if (const AstNodeDType* nodedtypep = VN_CAST(patp->keyp(), NodeDType)){ + // data_type: default_value + const string dtype = nodedtypep->dtypep()->prettyDTypeName(); + auto it = dtypemap.find(dtype); + if (it == dtypemap.end()) { + dtypemap.emplace(dtype, patp); + } + else { + // Override stored default_value + it->second = patp->cloneTree(false); + } + } + else { + // Undefined pattern + patp->keyp()->v3error( + "Assignment pattern key not supported/understood: " + << patp->keyp()->prettyTypeName()); + } } - if (memp && !patp) { - // Missing init elements, warn below - memp = nullptr; - patp = nullptr; - break; - } else if (!memp && patp) { - patp->v3error("Assignment pattern contains too many elements"); - memp = nullptr; - patp = nullptr; - break; - } else { - const std::pair ret = patmap.emplace(memp, patp); - if (!ret.second) { - patp->v3error("Assignment pattern contains duplicate entry: " - << VN_AS(patp->keyp(), Text)->text()); + else{ + // constant expr + if (memp) { + const std::pair ret = patmap.emplace(memp, patp); + if (!ret.second) { + patp->v3error("Assignment pattern contains duplicate entry: " + << VN_AS(patp->keyp(), Text)->text()); + } + memp = VN_AS(memp->nextp(), MemberDType); } } } while (false); + // Next - if (memp) memp = VN_AS(memp->nextp(), MemberDType); if (patp) patp = VN_AS(patp->nextp(), PatMember); } } @@ -3613,13 +3630,24 @@ private: AstPatMember* newpatp = nullptr; AstPatMember* patp = nullptr; if (it == patmap.end()) { - if (defaultp) { + const string memp_DType = memp->virtRefDTypep()->prettyDTypeName(); + const auto it2 = dtypemap.find(memp_DType); + if (it2 != dtypemap.end()) { + // default_value for data_type + patp = it2->second; + newpatp = patp->cloneTree(false); + patp = newpatp; + } + else if (defaultp) { + // default_value for any unassigned member yet newpatp = defaultp->cloneTree(false); patp = newpatp; } else { if (!VN_IS(vdtypep, UnionDType)) { nodep->v3error("Assignment pattern missed initializing elements: " - << memp->prettyTypeName()); + << memp->virtRefDTypep()->prettyDTypeName() + << " " + << memp->prettyName()); } } } else { diff --git a/src/verilog.y b/src/verilog.y index 82faf4a7d..79daad950 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -3544,6 +3544,7 @@ patternKey: // IEEE: merge structure_pattern_key, array_patt | yaFLOATNUM { $$ = new AstConst($1,AstConst::RealDouble(),$1); } | id { $$ = new AstText($1,*$1); } | strAsInt { $$ = $1; } + | simple_type { $$ = $1; } ; assignment_pattern: // ==IEEE: assignment_pattern diff --git a/test_regress/t/t_array_list_bad.out b/test_regress/t/t_array_list_bad.out index edf0db3e8..0eeb8a06f 100644 --- a/test_regress/t/t_array_list_bad.out +++ b/test_regress/t/t_array_list_bad.out @@ -1,4 +1,4 @@ -%Error: t/t_array_list_bad.v:38:25: Assignment pattern missed initializing elements: MEMBERDTYPE 't3' +%Error: t/t_array_list_bad.v:38:25: Assignment pattern missed initializing elements: logic t3 : ... In instance t 38 | test_out <= '{'0, '0}; | ^~ diff --git a/test_regress/t/t_structu_dataType_assignment.pl b/test_regress/t/t_structu_dataType_assignment.pl new file mode 100644 index 000000000..2cb5eeaff --- /dev/null +++ b/test_regress/t/t_structu_dataType_assignment.pl @@ -0,0 +1,21 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2021 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(simulator => 1); + +compile( + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_structu_dataType_assignment.v b/test_regress/t/t_structu_dataType_assignment.v new file mode 100644 index 000000000..2962a6e78 --- /dev/null +++ b/test_regress/t/t_structu_dataType_assignment.v @@ -0,0 +1,137 @@ +// DESCRIPTION: Verilator: Verilog Test module for specialized type default values +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Mostafa Gamal. +// SPDX-License-Identifier: CC0-1.0 + +/* verilator lint_off UNPACKED */ + +module top(); + + typedef struct { // IEEE 1800-2017 SV CH:5.10 + int a; + shortint b; + } ab_struct; + + typedef struct { // IEEE 1800-2017 SV CH:10.9.2 + int x; + int y; + } st_struct; + + typedef struct { // IEEE 1800-2017 SV CH:10.9.2 + logic [7:0] a; + bit b; + bit signed [31:0] c; + int s; + } sa_struct; + + + typedef struct { // IEEE 1800-2017 SV CH:10.9.2 + int A; + struct { + int B, C; + } BC1, BC2; + } DEF_struct; + + + // struct ab + ab_struct ab; + ab_struct abkey[1:0]; + + // struct st + st_struct st; + int k = 1; + + // struct sa + sa_struct sa; + + // struct DEF + DEF_struct DEF; + + initial begin; + // struct ab + ab = '{0, 0}; //constant member by position + if (ab.a != 0) $stop; + if (ab.b != 0) $stop; + + + ab = '{default: 0}; //default value + if (ab.a != 0) $stop; + if (ab.b != 0) $stop; + + + ab = '{int: 1, shortint: 0}; //data type and default value + if (ab.a != 1) $stop; + if (ab.b != 0) $stop; + + + abkey[1:0] = '{'{a:1, b:2}, '{int:2, shortint:3}}; // member: value & data_type: value + if (abkey[1].a != 1) $stop; + if (abkey[1].b != 2) $stop; + if (abkey[0].a != 2) $stop; + if (abkey[0].b != 3) $stop; + + + // struct st + st = '{1, 2+k}; //constant member by position + if (st.x != 1) $stop; + if (st.y != 2+k) $stop; + + st = '{x:2, y:3+k}; //member: value + if (st.x != 2) $stop; + if (st.y != 3+k) $stop; + + st = '{int:2, int:3+k}; //data_type: value override + if (st.x != 3+k) $stop; + if (st.y != 3+k) $stop; + + + // struct sa + sa = '{default:'1}; + if (sa.a != '1) $stop; + if (sa.b != '1) $stop; + if (sa.c != '1) $stop; + if (sa.s != '1) $stop; + + sa = '{default:'1, int: 5}; + if (sa.a != '1) $stop; + if (sa.b != '1) $stop; + if (sa.c != '1) $stop; + if (sa.s != 5) $stop; + + + sa = '{default:'1, int: 5, b: 0}; + if (sa.a != '1) $stop; + if (sa.b != 0) $stop; + if (sa.c != '1) $stop; + if (sa.s != 5) $stop; + + + // struct DEF + DEF = '{A:1, BC1:'{B:2, C:3}, BC2:'{B:4,C:5}}; + if (DEF.A != 1) $stop; + if (DEF.BC1.B != 2) $stop; + if (DEF.BC1.C != 3) $stop; + if (DEF.BC2.B != 4) $stop; + if (DEF.BC2.C != 5) $stop; + + + DEF = '{int:0, BC1:'{int:10}, BC2:'{default:5}}; + if (DEF.A != 0) $stop; + if (DEF.BC1.B != 10) $stop; + if (DEF.BC1.C != 10) $stop; + if (DEF.BC2.B != 5) $stop; + if (DEF.BC2.C != 5) $stop; + + DEF = '{default:1, BC1:'{int:10}, BC2:'{default:5}}; + if (DEF.A != 1) $stop; + if (DEF.BC1.B != 10) $stop; + if (DEF.BC1.C != 10) $stop; + if (DEF.BC2.B != 5) $stop; + if (DEF.BC2.C != 5) $stop; + + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule diff --git a/test_regress/t/t_structu_dataType_assignment_bad.out b/test_regress/t/t_structu_dataType_assignment_bad.out new file mode 100644 index 000000000..1b430ffd4 --- /dev/null +++ b/test_regress/t/t_structu_dataType_assignment_bad.out @@ -0,0 +1,5 @@ +%Error: t/t_structu_dataType_assignment_bad.v:19:26: Assignment pattern key not supported/understood: CONST '?32?sh1' + : ... In instance top + 19 | DEF_struct DEF_bad = '{1: 5, default: 10}; + | ^ +%Error: Exiting due to diff --git a/test_regress/t/t_structu_dataType_assignment_bad.pl b/test_regress/t/t_structu_dataType_assignment_bad.pl new file mode 100755 index 000000000..bec0388e9 --- /dev/null +++ b/test_regress/t/t_structu_dataType_assignment_bad.pl @@ -0,0 +1,21 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2021 by Wilson Snyder. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(simulator => 1); + + +compile( + fails => 1, + expect_filename => $Self->{golden_filename}, + ); + + +ok(1); +1; diff --git a/test_regress/t/t_structu_dataType_assignment_bad.v b/test_regress/t/t_structu_dataType_assignment_bad.v new file mode 100644 index 000000000..6ab523bbf --- /dev/null +++ b/test_regress/t/t_structu_dataType_assignment_bad.v @@ -0,0 +1,21 @@ +// DESCRIPTION: Verilator: Verilog Test module for specialized type default values +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Mostafa Gamal. +// SPDX-License-Identifier: CC0-1.0 + +/* verilator lint_off UNPACKED */ + +module top(); + + + typedef struct { // IEEE 1800-2017 SV CH:10.9.2 + int A; + struct { + int B, C; + } BC1, BC2; + } DEF_struct; + + DEF_struct DEF_bad = '{1: 5, default: 10}; + +endmodule From e871cd8a4438432b9641d56b71c35fd2bc2e961e Mon Sep 17 00:00:00 2001 From: github action Date: Mon, 25 Jul 2022 21:47:29 +0000 Subject: [PATCH 07/11] Apply 'make format' --- src/V3Width.cpp | 35 +++++++++---------- .../t/t_structu_dataType_assignment.pl | 0 2 files changed, 16 insertions(+), 19 deletions(-) mode change 100644 => 100755 test_regress/t/t_structu_dataType_assignment.pl diff --git a/src/V3Width.cpp b/src/V3Width.cpp index 9ce0b5b4c..237675b4a 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -3560,7 +3560,8 @@ private: // determine the dtypep for that PatMember's value, and then // width the initial value appropriately. using PatMap = std::map; // Store member: value - using DTypeMap = std::map; // Store data_type: default_value + using DTypeMap + = std::map; // Store data_type: default_value PatMap patmap; DTypeMap dtypemap; { @@ -3575,44 +3576,42 @@ private: memp = vdtypep->findMember(textp->text()); if (!memp) { patp->keyp()->v3error("Assignment pattern key '" - << textp->text() - << "' not found as member"); + << textp->text() << "' not found as member"); break; } else { - const std::pair ret = patmap.emplace(memp, patp); + const std::pair ret + = patmap.emplace(memp, patp); if (!ret.second) { patp->v3error("Assignment pattern contains duplicate entry: " - << VN_AS(patp->keyp(), Text)->text()); + << VN_AS(patp->keyp(), Text)->text()); } memp = VN_AS(memp->nextp(), MemberDType); } - } - else if (const AstNodeDType* nodedtypep = VN_CAST(patp->keyp(), NodeDType)){ + } else if (const AstNodeDType* nodedtypep + = VN_CAST(patp->keyp(), NodeDType)) { // data_type: default_value const string dtype = nodedtypep->dtypep()->prettyDTypeName(); auto it = dtypemap.find(dtype); if (it == dtypemap.end()) { dtypemap.emplace(dtype, patp); - } - else { + } else { // Override stored default_value it->second = patp->cloneTree(false); } - } - else { + } else { // Undefined pattern patp->keyp()->v3error( "Assignment pattern key not supported/understood: " << patp->keyp()->prettyTypeName()); } - } - else{ + } else { // constant expr if (memp) { - const std::pair ret = patmap.emplace(memp, patp); + const std::pair ret + = patmap.emplace(memp, patp); if (!ret.second) { patp->v3error("Assignment pattern contains duplicate entry: " - << VN_AS(patp->keyp(), Text)->text()); + << VN_AS(patp->keyp(), Text)->text()); } memp = VN_AS(memp->nextp(), MemberDType); } @@ -3637,16 +3636,14 @@ private: patp = it2->second; newpatp = patp->cloneTree(false); patp = newpatp; - } - else if (defaultp) { + } else if (defaultp) { // default_value for any unassigned member yet newpatp = defaultp->cloneTree(false); patp = newpatp; } else { if (!VN_IS(vdtypep, UnionDType)) { nodep->v3error("Assignment pattern missed initializing elements: " - << memp->virtRefDTypep()->prettyDTypeName() - << " " + << memp->virtRefDTypep()->prettyDTypeName() << " " << memp->prettyName()); } } diff --git a/test_regress/t/t_structu_dataType_assignment.pl b/test_regress/t/t_structu_dataType_assignment.pl old mode 100644 new mode 100755 From a5ddd10e31b089637f1e85712f3cdef3bb6025d9 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Wed, 27 Jul 2022 10:45:33 +0100 Subject: [PATCH 08/11] Tests: compare VCD files both ways vcddiff is a bit broken, and sometimes 'vcddiff a b' fails while the files are indeed equivalent. There is a chance however that 'vcddif b a' will succeed in this case, so compare trace files both ways when checking test results and claim success if vcddiff succeeds in at least one direction. --- test_regress/driver.pl | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test_regress/driver.pl b/test_regress/driver.pl index ae0ed4f36..22a2a67f8 100755 --- a/test_regress/driver.pl +++ b/test_regress/driver.pl @@ -2259,10 +2259,15 @@ sub vcd_identical { print "\t$cmd\n" if $::Debug; $out = `$cmd`; if ($? != 0 || $out ne '') { - print $out; - $self->error("VCD miscompares $fn1 $fn2\n"); - $self->copy_if_golden($fn1, $fn2); - return 0; + $cmd = qq{vcddiff "$fn2" "$fn1"}; + print "\t$cmd\n" if $::Debug; + $out = `$cmd`; + if ($? != 0 || $out ne '') { + print $out; + $self->error("VCD miscompares $fn2 $fn1\n"); + $self->copy_if_golden($fn1, $fn2); + return 0; + } } } { From 2a87387eb393f2a0ce434796311688e757fd451e Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Thu, 28 Jul 2022 08:41:01 -0400 Subject: [PATCH 09/11] Documentation fixes (#3514) --- docs/guide/files.rst | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/guide/files.rst b/docs/guide/files.rst index 796a4c756..a71214983 100644 --- a/docs/guide/files.rst +++ b/docs/guide/files.rst @@ -50,7 +50,7 @@ For --cc/--sc, it creates: - Make include file with class names (from --make gmake) * - *{prefix}*\ _hier.mk - Make file for hierarchy blocks (from --make gmake) - * - *{prefix|*\ _hierMkArgs.f + * - *{prefix}*\ _hierMkArgs.f - Arguments for hierarchical Verilation (from --make gmake) * - *{prefix}*\ _hierCMakeArgs.f - Arguments for hierarchical Verilation (from --make cmake) @@ -62,13 +62,17 @@ For --cc/--sc, it creates: - Top level (SystemVerilog $root) internal header file * - *{prefix}*\ ___024root.cpp - Top level (SystemVerilog $root) internal C++ file - * - *{prefix}*___024root*{__n}*\ .cpp - - Additional top level internal C++ files (from --output-split) + * - *{prefix}*\ ___024root\ *{__n}*\ .cpp + - Additional top level internal C++ files + * - *{prefix}*\ ___024root\ *{__DepSet_hash__n}*\ .cpp + - Additional top level internal C++ files (hashed to reduce build times) * - *{prefix}*\ ___024root__Slow\ *{__n}*\ .cpp - Infrequent cold routines - * - *{prefix}*\ ___024root__Trace{__n}*\ .cpp + * - *{prefix}*\ ___024root\ *{__DepSet_hash__n}*\ .cpp + - Infrequent cold routines (hashed to reduce build times) + * - *{prefix}*\ ___024root__Trace\ *{__n}*\ .cpp - Wave file generation code (from --trace) - * - *{prefix}*\ ___024root__Trace__Slow{__n}*\ .cpp + * - *{prefix}*\ ___024root__Trace__Slow\ *{__n}*\ .cpp - Wave file generation code (from --trace) * - *{prefix}*\ __Dpi.h - DPI import and export declarations (from --dpi) @@ -87,7 +91,9 @@ For --cc/--sc, it creates: * - *{prefix}{each_verilog_module}*\ .cpp - Lower level internal C++ files * - *{prefix}{each_verilog_module}{__n}*\ .cpp - - Additional lower C++ files (from --output-split) + - Additional lower C++ files + * - *{prefix}{each_verilog_module}{__DepSet_hash__n}*\ .cpp + - Additional lower C++ files (hased to reduce build times) For --hierarchy mode, it creates: From 574dbfded1074b37e5d89272ca345dc3c067bf92 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Thu, 28 Jul 2022 12:54:28 +0100 Subject: [PATCH 10/11] V3MergeCond: Fix incorrect merge of assignments to the condition --- src/V3MergeCond.cpp | 2 ++ test_regress/t/t_merge_cond_no_extend.pl | 20 ++++++++++++++++++++ test_regress/t/t_merge_cond_no_extend.v | 22 ++++++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100755 test_regress/t/t_merge_cond_no_extend.pl create mode 100644 test_regress/t/t_merge_cond_no_extend.v diff --git a/src/V3MergeCond.cpp b/src/V3MergeCond.cpp index 210d34ca6..bb4251cbb 100644 --- a/src/V3MergeCond.cpp +++ b/src/V3MergeCond.cpp @@ -790,6 +790,8 @@ private: // otherwise end the current merge. Return ture if added, false if ended merge. bool addIfHelpfulElseEndMerge(AstNodeStmt* nodep) { UASSERT_OBJ(m_mgFirstp, nodep, "List must be open"); + if (!checkOrMakeMergeable(nodep)) return false; + if (!m_mgFirstp) return false; // If 'checkOrMakeMergeable' closed the list if (m_mgNextp == nodep) { if (isSimplifiableNode(nodep)) { if (addToList(nodep, nullptr)) return true; diff --git a/test_regress/t/t_merge_cond_no_extend.pl b/test_regress/t/t_merge_cond_no_extend.pl new file mode 100755 index 000000000..5fd0b644e --- /dev/null +++ b/test_regress/t/t_merge_cond_no_extend.pl @@ -0,0 +1,20 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2022 by Geza Lore. 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-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(vlt_all => 1); + +compile( + verilator_flags2 => ["--stats"], + ); + +file_grep($Self->{stats}, qr/Optimizations, MergeCond merges\s+(\d+)/i, 0); + +ok(1); +1; diff --git a/test_regress/t/t_merge_cond_no_extend.v b/test_regress/t/t_merge_cond_no_extend.v new file mode 100644 index 000000000..ede818af3 --- /dev/null +++ b/test_regress/t/t_merge_cond_no_extend.v @@ -0,0 +1,22 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Geza Lore. +// SPDX-License-Identifier: CC0-1.0 + +module t ( + input wire clk, + input wire [7:0] i, + input wire a, + output reg [7:0] o +); + + reg cond = 0; + + always @(posedge clk) begin + if (cond) o = i; + cond = a; + if (cond) o = ~i; + end + +endmodule From 1f9323d0868d2fb0f9c745fdbf74154239604856 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Fri, 29 Jul 2022 07:05:04 +0900 Subject: [PATCH 11/11] Set correct dtype in replaceShiftSame() (#3520) * Tests: Add a test to reproduce bug3399 * Fix3399. Set the correct dtype in replaceShiftSame(). * Tests: update stats. * Update Changes --- Changes | 1 + src/V3Const.cpp | 1 + test_regress/t/t_const_opt.pl | 2 +- test_regress/t/t_const_opt.v | 30 ++++++++++++++++++++++++++++-- 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/Changes b/Changes index 9b7dd96e8..6cfd4b984 100644 --- a/Changes +++ b/Changes @@ -17,6 +17,7 @@ Verilator 4.225 devel * Fix empty string arguments to display (#3484). [Grulfen] * Fix table misoptimizing away display (#3488). [Stefan Post] * Fix wrong bit op tree optimization (#3509). [Nathan Graybeal] +* Fix incorrect tristate logic (#3399) [shareefj, Vighnesh Iyer] Verilator 4.224 2022-06-19 diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 6f543fdba..372757419 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -1764,6 +1764,7 @@ private: lp->rhsp(lrp); nodep->lhsp(llp); nodep->rhsp(rlp); + nodep->dtypep(llp->dtypep()); // dtype of Biop is before shift. VL_DO_DANGLING(rp->deleteTree(), rp); VL_DO_DANGLING(rrp->deleteTree(), rrp); // nodep->dumpTree(cout, " repShiftSame_new: "); diff --git a/test_regress/t/t_const_opt.pl b/test_regress/t/t_const_opt.pl index 837b5f74f..36f064cb4 100755 --- a/test_regress/t/t_const_opt.pl +++ b/test_regress/t/t_const_opt.pl @@ -19,7 +19,7 @@ execute( ); if ($Self->{vlt}) { - file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 14); + file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 15); } ok(1); 1; diff --git a/test_regress/t/t_const_opt.v b/test_regress/t/t_const_opt.v index 559477475..d1c545a61 100644 --- a/test_regress/t/t_const_opt.v +++ b/test_regress/t/t_const_opt.v @@ -62,7 +62,7 @@ module t(/*AUTOARG*/ $write("[%0t] cyc==%0d crc=%x sum=%x\n", $time, cyc, crc, sum); if (crc !== 64'hc77bb9b3784ea091) $stop; // What checksum will we end up with (above print should match) -`define EXPECTED_SUM 64'hde21e019a3e12039 +`define EXPECTED_SUM 64'h9366e49d91bfe942 if (sum !== `EXPECTED_SUM) $stop; $write("*-* All Finished *-*\n"); @@ -88,10 +88,12 @@ module Test(/*AUTOARG*/ logic bug3445_out; logic bug3470_out; logic bug3509_out; + wire bug3399_out0; + wire bug3399_out1; output logic o; - logic [9:0] tmp; + logic [11:0] tmp; assign o = ^tmp; always_ff @(posedge clk) begin @@ -117,6 +119,8 @@ module Test(/*AUTOARG*/ tmp[7] <= bug3445_out; tmp[8] <= bug3470_out; tmp[9] <= bug3509_out; + tmp[10]<= bug3399_out0; + tmp[11]<= bug3399_out1; end bug3182 i_bug3182(.in(d[4:0]), .out(bug3182_out)); @@ -124,6 +128,7 @@ module Test(/*AUTOARG*/ bug3445 i_bug3445(.clk(clk), .in(d), .out(bug3445_out)); bug3470 i_bug3470(.clk(clk), .in(d), .out(bug3470_out)); bug3509 i_bug3509(.clk(clk), .in(d), .out(bug3509_out)); + bug3399 i_bug3399(.clk(clk), .in(d), .out0(bug3399_out0), .out1(bug3399_out1)); endmodule @@ -289,3 +294,24 @@ module bug3509(input wire clk, input wire [31:0] in, output reg out); end end endmodule + +// Bug3399 +// replaceShiftSame() in V3Const.cpp optimizes +// Or(Shift(ll,CONSTlr),Shift(rl,CONSTrr==lr)) -> Shift(Or(ll,rl),CONSTlr) +// (Or/And may also be reversed) +// +// dtype of Or after the transformation must be as same as ll and rl, but was dtype of Or BEFORE transformation. +// When the result of Shift was 1 bit width, bit op tree optimization +// optimized the tree even though the graph needs more width. +// Remember that the target of bit op tree optimization is 1 bit width. +module bug3399(input wire clk, input wire [31:0] in, inout wire out0, inout wire out1); + logic [1:0] driver = '0; + logic [1:0] d; + always_ff @(posedge clk) begin + driver <= 2'b11; + d <= in[1:0]; + end + + assign out0 = driver[0] ? d[0] : 1'bz; + assign out1 = driver[1] ? d[1] : 1'bz; +endmodule