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