Mario1159/verilator
1
0
Fork 0
forked from github/verilator
Code Pull Requests Releases 1 Activity

Fix wrong result of bit op tree optimization #3509 (#3516)

Browse Source 
* 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
This commit is contained in:
Yutetsu TAKATSUKASA 2022-07-24 19:54:37 +09:00 committed by GitHub
parent e0b61ceabd
commit 60eab3eb8c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 93 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Changes
View File

@ -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

45
src/V3Const.cpp
View File

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

9
test_regress/t/t_const_no_opt.pl
View File

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

56
test_regress/t/t_const_opt.v
View File

@ -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