From 1a056f6db9e6ed74f0912ec30b8031c4336d51a6 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sun, 15 May 2022 16:34:04 +0100 Subject: [PATCH] Fix invalid conditional merging when starting at 'c = c ? a : b' Fixes #3409. --- src/V3MergeCond.cpp | 26 ++++--- test_regress/t/t_merge_cond_bug_3409.pl | 29 ++++++++ test_regress/t/t_merge_cond_bug_3409.v | 93 +++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 9 deletions(-) create mode 100755 test_regress/t/t_merge_cond_bug_3409.pl create mode 100644 test_regress/t/t_merge_cond_bug_3409.v diff --git a/src/V3MergeCond.cpp b/src/V3MergeCond.cpp index 77b2ea935..673326f27 100644 --- a/src/V3MergeCond.cpp +++ b/src/V3MergeCond.cpp @@ -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([](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([](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 diff --git a/test_regress/t/t_merge_cond_bug_3409.pl b/test_regress/t/t_merge_cond_bug_3409.pl new file mode 100755 index 000000000..bb6c1fab4 --- /dev/null +++ b/test_regress/t/t_merge_cond_bug_3409.pl @@ -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; diff --git a/test_regress/t/t_merge_cond_bug_3409.v b/test_regress/t/t_merge_cond_bug_3409.v new file mode 100644 index 000000000..71e0ccf48 --- /dev/null +++ b/test_regress/t/t_merge_cond_bug_3409.v @@ -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