luke
/
verilator
mirror of https://github.com/verilator/verilator.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix #3445 of BitOpTreeOpt (#3453) 

* Tests: Check BitOpTree statistics in t_const_opt.

* Tests: Add a test to reproduce #3445

* Fix #3445. Don't forget LSB of frozen node in BitOpTreeOpt.

* Apply suggestions from code review

Co-authored-by: Geza Lore <gezalore@gmail.com>
This commit is contained in:
Yutetsu TAKATSUKASA 2022-05-30 19:33:06 +09:00 committed by GitHub
parent b51f887567
commit 26b7452178
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 87 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
src/V3Const.cpp
View File

@ -299,7 +299,8 @@ class ConstBitOpTreeVisitor final : public VNVisitor {
LeafInfo* m_leafp = nullptr; // AstConst or AstVarRef that currently looking for
const AstNode* const m_rootp; // Root of this AST subtree
std::vector<AstNode*> m_frozenNodes; // Nodes that cannot be optimized
std::vector<std::pair<AstNode*, int>>
m_frozenNodes; // Nodes that cannot be optimized, int is lsb
std::vector<BitPolarityEntry> m_bitPolarities; // Polarity of bits found during iterate()
std::vector<std::unique_ptr<VarInfo>> m_varInfos; // VarInfo for each variable, [0] is nullptr
@ -487,7 +488,7 @@ class ConstBitOpTreeVisitor final : public VNVisitor {
restorer.restoreNow();
// Reach past a cast then add to frozen nodes to be added to final reduction
if (const AstCCast* const castp = VN_CAST(opp, CCast)) opp = castp->lhsp();
m_frozenNodes.push_back(opp);
m_frozenNodes.emplace_back(opp, m_lsb);
m_failed = origFailed;
continue;
}
@ -652,17 +653,21 @@ public:
}
}
std::map<int, std::vector<AstNode*>> frozenNodes; // Group by LSB
// Check if frozen terms are clean or not
for (AstNode* const termp : visitor.m_frozenNodes) {
for (const std::pair<AstNode*, int>& termAndLsb : visitor.m_frozenNodes) {
AstNode* const termp = termAndLsb.first;
// Comparison operators are clean
if (VN_IS(termp, Eq) || VN_IS(termp, Neq) || VN_IS(termp, Lt) || VN_IS(termp, Lte)
|| VN_IS(termp, Gt) || VN_IS(termp, Gte)) {
if ((VN_IS(termp, Eq) || VN_IS(termp, Neq) || VN_IS(termp, Lt) || VN_IS(termp, Lte)
|| VN_IS(termp, Gt) || VN_IS(termp, Gte))
&& termAndLsb.second == 0) {
hasCleanTerm = true;
} else {
// Otherwise, conservatively assume the frozen term is dirty
hasDirtyTerm = true;
UINFO(9, "Dirty frozen term: " << termp << endl);
}
frozenNodes[termAndLsb.second].push_back(termp);
}
// Figure out if a final negation is required
@ -672,7 +677,11 @@ public:
const bool needsCleaning = visitor.isAndTree() ? !hasCleanTerm : hasDirtyTerm;
// Add size of reduction tree to op count
resultOps += termps.size() + visitor.m_frozenNodes.size() - 1;
resultOps += termps.size() - 1;
for (const auto& lsbAndNodes : frozenNodes) {
if (lsbAndNodes.first > 0) ++resultOps; // Needs AstShiftR
resultOps += lsbAndNodes.second.size();
}
// Add final polarity flip in Xor tree
if (needsFlip) ++resultOps;
// Add final cleaning AND
@ -681,7 +690,9 @@ public:
if (debug() >= 9) { // LCOV_EXCL_START
cout << "Bitop tree considered: " << endl;
for (AstNode* const termp : termps) termp->dumpTree("Reduced term: ");
for (AstNode* const termp : visitor.m_frozenNodes) termp->dumpTree("Frozen term: ");
for (const std::pair<AstNode*, int>& termp : visitor.m_frozenNodes)
termp.first->dumpTree("Frozen term with lsb " + std::to_string(termp.second)
+ ": ");
cout << "Needs flipping: " << needsFlip << endl;
cout << "Needs cleaning: " << needsCleaning << endl;
cout << "Size: " << resultOps << " input size: " << visitor.m_ops << endl;
@ -724,8 +735,19 @@ public:
resultp = reduce(resultp, termp);
}
// Add any frozen terms to the reduction
for (AstNode* const frozenp : visitor.m_frozenNodes) {
resultp = reduce(resultp, frozenp->unlinkFrBack());
for (auto&& lsbAndNodes : frozenNodes) {
AstNode* termp = nullptr;
for (AstNode* const itemp : lsbAndNodes.second) {
termp = reduce(termp, itemp->unlinkFrBack());
}
if (lsbAndNodes.first > 0) { // LSB is not 0, so shiftR
AstNodeDType* const dtypep = termp->dtypep();
termp = new AstShiftR{termp->fileline(), termp,
new AstConst(termp->fileline(), AstConst::WidthedValue{},
termp->width(), lsbAndNodes.first)};
termp->dtypep(dtypep);
}
resultp = reduce(resultp, termp);
}
// Set width of masks to expected result width. This is required to prevent later removal

3
test_regress/t/t_const_opt.pl
View File

@ -18,5 +18,8 @@ execute(
check_finished => 1,
);
if ($Self->{vlt}) {
file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 10);
}
ok(1);
1;

55
test_regress/t/t_const_opt.v
View File

@ -57,7 +57,8 @@ 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'hcae926ece668f35d
`define EXPECTED_SUM 64'h194081987b76c71c
if (sum !== `EXPECTED_SUM) $stop;
$write("*-* All Finished *-*\n");
$finish;
@ -79,10 +80,11 @@ module Test(/*AUTOARG*/
logic d0, d1, d2, d3, d4, d5, d6, d7;
logic bug3182_out;
logic bug3197_out;
logic bug3445_out;
output logic o;
logic [6:0] tmp;
logic [7:0] tmp;
assign o = ^tmp;
always_ff @(posedge clk) begin
@ -105,10 +107,12 @@ module Test(/*AUTOARG*/
tmp[4] <= i[0] & (i[1] & (i[2] & (i[3] | d[4]))); // ConstBitOpTreeVisitor::m_frozenNodes
tmp[5] <= bug3182_out;
tmp[6] <= bug3197_out;
tmp[7] <= bug3445_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));
endmodule
@ -140,3 +144,50 @@ module bug3197(input wire clk, input wire [31:0] in, output out);
wire tmp0 = (|d[38:0]);
assign out = (d[39] | tmp0);
endmodule
// Bug #3445
// An unoptimized node is kept as frozen node, but its LSB were not saved.
// AST of RHS of result0 looks as below:
// AND(SHIFTR(AND(WORDSEL(ARRAYSEL(VARREF)), WORDSEL(ARRAYSEL(VARREF)))), 32'd11)
// ~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
// Two of WORDSELs are frozen nodes. They are under SHIFTR of 11 bits.
//
// Fixing #3445 needs to
// 1. Take AstShiftR into op count when diciding optimizable or not
// (result0 in the test)
// 2. Insert AstShiftR if LSB of the frozen node is not 0 (result1 in the test)
module bug3445(input wire clk, input wire [31:0] in, output wire out);
logic [127:0] d;
always_ff @(posedge clk)
d <= {d[95:0], in};
typedef struct packed {
logic a;
logic [ 2:0] b;
logic [ 2:0] c;
logic [ 1:0] d;
logic [ 7:0] e;
logic [31:0] f;
logic [ 3:0] g;
logic [31:0] h;
logic i;
logic [41:0] j;
} packed_struct;
packed_struct st[2];
always_ff @(posedge clk) begin
st[0] <= d;
st[1] <= st[0];
end
logic result0, result1;
always_ff @(posedge clk) begin
// Cannot optimize further.
result0 <= (st[0].g[0] & st[0].h[0]) & (in[0] == 1'b0);
// There are redundant !in[0] terms. They should be simplified.
result1 <= (!in[0] & (st[1].g[0] & st[1].h[0])) & ((in[0] == 1'b0) & !in[0]);
end
assign out = result0 ^ result1;
endmodule