From b08c694cd626e349f4bad8048ab946485c5ba6b0 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Sat, 6 Nov 2021 12:31:50 +0900 Subject: [PATCH] Fix wrong bit op tree optimization (#3185) * Add a test to reproduce bug3182. Run the same HDL with -Oo to confirm the result is same. * Hopefully fix #3182. The result can be 0 only when polarity is true (no AstNot is found during traversal). --- src/V3Const.cpp | 2 +- test_regress/t/t_const_no_opt.pl | 24 ++++++++++++++++++++++++ test_regress/t/t_const_opt.cpp | 13 +++++++++++++ test_regress/t/t_const_opt.pl | 2 +- test_regress/t/t_const_opt.v | 28 ++++++++++++++++++++++++++-- 5 files changed, 65 insertions(+), 4 deletions(-) create mode 100755 test_regress/t/t_const_no_opt.pl create mode 100644 test_regress/t/t_const_opt.cpp diff --git a/src/V3Const.cpp b/src/V3Const.cpp index 259cc7417..a1277871e 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -537,7 +537,7 @@ class ConstBitOpTreeVisitor final : public AstNVisitor { if (leafInfo.m_lsb < leafInfo.width()) { m_bitPolarities.emplace_back(leafInfo, isXorTree() || leafInfo.m_polarity, leafInfo.m_lsb); - } else if (isAndTree()) { + } else if (isAndTree() && leafInfo.m_polarity) { // If there is a constant 0 term in an And tree, we must include it. Fudge // this by adding a bit with both polarities, which will simplify to zero m_bitPolarities.emplace_back(leafInfo, true, 0); diff --git a/test_regress/t/t_const_no_opt.pl b/test_regress/t/t_const_no_opt.pl new file mode 100755 index 000000000..33be39810 --- /dev/null +++ b/test_regress/t/t_const_no_opt.pl @@ -0,0 +1,24 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. 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); +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", "-Oo", "$Self->{t_dir}/t_const_opt.cpp"], + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_const_opt.cpp b/test_regress/t/t_const_opt.cpp new file mode 100644 index 000000000..116148dfb --- /dev/null +++ b/test_regress/t/t_const_opt.cpp @@ -0,0 +1,13 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// +// Copyright 2021 by Yutetsu TAKATSUKASA. 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 +// +//************************************************************************* + +// This function is used to introduce dependency and disturb optimization +extern "C" int fake_dependency() { return 0; } diff --git a/test_regress/t/t_const_opt.pl b/test_regress/t/t_const_opt.pl index ee51e8030..26143eb57 100755 --- a/test_regress/t/t_const_opt.pl +++ b/test_regress/t/t_const_opt.pl @@ -11,7 +11,7 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(simulator => 1); compile( - verilator_flags2 => ["-Wno-UNOPTTHREADS", "--stats"], + verilator_flags2 => ["-Wno-UNOPTTHREADS", "--stats", "$Self->{t_dir}/$Self->{name}.cpp"], ); execute( diff --git a/test_regress/t/t_const_opt.v b/test_regress/t/t_const_opt.v index e8f31dfe1..2b953aa8d 100644 --- a/test_regress/t/t_const_opt.v +++ b/test_regress/t/t_const_opt.v @@ -57,7 +57,7 @@ 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'he78be35df15ae0ab +`define EXPECTED_SUM 64'ha916d9291821c6e0 if (sum !== `EXPECTED_SUM) $stop; $write("*-* All Finished *-*\n"); $finish; @@ -77,10 +77,11 @@ module Test(/*AUTOARG*/ input [31:0] i; logic [31:0] d; logic d0, d1, d2, d3, d4, d5, d6, d7; + logic bug3182_out; output logic o; - logic [4:0] tmp; + logic [5:0] tmp; assign o = ^tmp; always_ff @(posedge clk) begin @@ -101,6 +102,29 @@ module Test(/*AUTOARG*/ tmp[2] <= ((d0 & d1) | (d0 & d2))^ ((d3 & d4) | (d5 & d4)); // replaceAndOr() tmp[3] <= d0 <-> d1; // replaceLogEq() tmp[4] <= i[0] & (i[1] & (i[2] & (i[3] | d[4]))); // ConstBitOpTreeVisitor::m_frozenNodes + tmp[5] <= bug3182_out; end + bug3182 i_bug3182(.in(d[4:0]), .out(bug3182_out)); + +endmodule + +module bug3182(in, out); + input wire [4:0] in; + output wire out; + + // This function always returns 0, so safe to take bitwise OR with any value. + // Calling this function stops constant folding as Verialtor does not know + // what this function returns. + import "DPI-C" context function int fake_dependency(); + + logic [4:0] bit_source; + + /* verilator lint_off WIDTH */ + always @(in) + bit_source = fake_dependency() | in; + + wire [5:0] tmp = bit_source; // V3Gate should inline this + wire out = ~(tmp >> 5) & (bit_source == 5'd10); + /* verilator lint_on WIDTH */ endmodule