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

Fix invalid conditional merging when starting at 'c = c ? a : b' 

Fixes #3409.
This commit is contained in:
Geza Lore 2022-05-15 16:34:04 +01:00
parent e018eb7bac
commit 1a056f6db9
3 changed files with 139 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/V3MergeCond.cpp
View File

@ -382,15 +382,24 @@ private:
return false;
}
void addToList(AstNode* nodep, AstNode* condp, int line) {
bool addToList(AstNode* nodep, AstNode* condp, int line) {
// Set up head of new list if node is first in list
if (!m_mgFirstp) {
UASSERT_OBJ(condp, nodep, "Cannot start new list without condition " << line);
// Mark variable references in the condition
condp->foreach<AstVarRef>([](const AstVarRef* nodep) { nodep->varp()->user1(1); });
// Now check again if mergeable. We need this to pick up assignments to conditions,
// e.g.: 'c = c ? a : b' at the beginning of the list, which is in fact not mergeable
// because it updates the condition. We simply bail on these.
if (m_checkMergeable(nodep) != Mergeable::YES) {
// Clear marked variables
AstNode::user1ClearTree();
// We did not add to the list
return false;
}
m_mgFirstp = nodep;
m_mgCondp = condp;
m_listLenght = 0;
// Mark variable references in the condition
condp->foreach<AstVarRef>([](const AstVarRef* nodep) { nodep->varp()->user1(1); });
// Add any preceding nodes to the list that would allow us to extend the merge range
for (;;) {
AstNode* const backp = m_mgFirstp->backp();
@ -416,6 +425,8 @@ private:
m_mgNextp = nodep->nextp();
// If last under parent, done with current list
if (!m_mgNextp) mergeEnd(__LINE__);
// We did add to the list
return true;
}
// If this node is the next expected node and is helpful to add to the list, do so,
@ -424,13 +435,10 @@ private:
UASSERT_OBJ(m_mgFirstp, nodep, "List must be open");
if (m_mgNextp == nodep) {
if (isSimplifiableNode(nodep)) {
addToList(nodep, nullptr, __LINE__);
return true;
}
if (isCheapNode(nodep)) {
if (addToList(nodep, nullptr, __LINE__)) return true;
} else if (isCheapNode(nodep)) {
nodep->user2(1);
addToList(nodep, nullptr, __LINE__);
return true;
if (addToList(nodep, nullptr, __LINE__)) return true;
}
}
// Not added to list, so we are done with the current list

29
test_regress/t/t_merge_cond_bug_3409.pl Executable file
View File

@ -0,0 +1,29 @@
#!/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(simulator => 1);
compile(
verilator_flags2=> ["--stats"]
);
execute();
if ($Self->{vlt_all}) {
file_grep($Self->{stats}, qr/Optimizations, MergeCond merges\s+(\d+)/i,
0);
file_grep($Self->{stats}, qr/Optimizations, MergeCond merged items\s+(\d+)/i,
0);
file_grep($Self->{stats}, qr/Optimizations, MergeCond longest merge\s+(\d+)/i,
0);
}
ok(1);
1;

93
test_regress/t/t_merge_cond_bug_3409.v Normal file
View File

@ -0,0 +1,93 @@
// DESCRIPTION: Verilator: Verilog Test module
//
// This file ONLY is placed under the Creative Commons Public Domain, for
// any use, without warranty, 2022 by Raynard Qiao.
// SPDX-License-Identifier: CC0-1.0
module t(/*AUTOARG*/
// Inputs
clk
);
input clk;
integer cyc = 0;
reg [63:0] crc;
reg [63:0] sum;
// Take CRC data and apply to testblock inputs
wire [3:0] din = crc[3:0];
/*AUTOWIRE*/
// Beginning of automatic wires (for undeclared instantiated-module outputs)
wire row_found; // From test of Test.v
wire [1:0] row_idx; // From test of Test.v
// End of automatics
Test test(/*AUTOINST*/
// Outputs
.row_idx (row_idx[1:0]),
.row_found (row_found),
// Inputs
.din (din));
// Aggregate outputs into a single result vector
wire [63:0] result = {48'b0, din, 7'b0, row_found, 2'b0, row_idx};
// Test loop
always @ (posedge clk) begin
`ifdef TEST_VERBOSE
$write("[%0t] cyc==%0d crc=%x result=%x\n", $time, cyc, crc, result);
`endif
cyc <= cyc + 1;
crc <= {crc[62:0], crc[63] ^ crc[2] ^ crc[0]};
sum <= result ^ {sum[62:0], sum[63] ^ sum[2] ^ sum[0]};
if (cyc == 0) begin
// Setup
crc <= 64'h5aef0c8d_d70a4497;
sum <= '0;
end
else if (cyc < 10) begin
sum <= '0;
end
else if (cyc == 99) begin
$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'h8b61595b704e511f
if (sum !== `EXPECTED_SUM) $stop;
$write("*-* All Finished *-*\n");
$finish;
end
end
endmodule
module Test(/*AUTOARG*/
// Outputs
row_idx, row_found,
// Inputs
din
);
input din;
output [1:0] row_idx;
output row_found;
reg [3:0] din;
reg [3:0] wide_din;
reg row_found;
reg [1:0] row_idx;
always_comb begin
integer x;
row_idx = {2{1'b0}};
row_found = 1'b0;
// Bug #3409: After unrolling, these conditionals should not be merged
// as row_found is assigned.
for (x = 0; $unsigned(x) < 4; x = x + 1) begin
row_idx = !row_found ? x[1:0] : row_idx;
row_found = !row_found ? din[x] : row_found;
end
end
endmodule