From 45edcbb03ed9e0820d1b483a6c6199b98f81ae48 Mon Sep 17 00:00:00 2001 From: Ryszard Rozak Date: Tue, 17 Oct 2023 13:38:45 +0200 Subject: [PATCH] Fix logical expressions with class objects - caching in v3Const (#4552) --- src/V3AstNodeOther.h | 4 +++ src/V3Const.cpp | 48 +++++++++++++++++++++++-- src/V3Task.cpp | 1 + test_regress/t/t_class_short_circuit.pl | 21 +++++++++++ test_regress/t/t_class_short_circuit.v | 33 +++++++++++++++++ test_regress/t/t_const_opt.pl | 2 +- 6 files changed, 105 insertions(+), 4 deletions(-) create mode 100755 test_regress/t/t_class_short_circuit.pl create mode 100644 test_regress/t/t_class_short_circuit.v diff --git a/src/V3AstNodeOther.h b/src/V3AstNodeOther.h index 2fea7ff71..568a0f5b8 100644 --- a/src/V3AstNodeOther.h +++ b/src/V3AstNodeOther.h @@ -609,6 +609,7 @@ private: bool m_dpiImportWrapper : 1; // Wrapper for invoking DPI import prototype from generated code bool m_dpiTraceInit : 1; // DPI trace_init bool m_needProcess : 1; // Needs access to VlProcess of the caller + bool m_recursive : 1; // Recursive or part of recursion public: AstCFunc(FileLine* fl, const string& name, AstScope* scopep, const string& rtnType = "") : ASTGEN_SUPER_CFunc(fl) { @@ -637,6 +638,7 @@ public: m_dpiImportPrototype = false; m_dpiImportWrapper = false; m_dpiTraceInit = false; + m_recursive = false; } ASTGEN_MEMBERS_AstCFunc; string name() const override VL_MT_STABLE { return m_name; } @@ -716,6 +718,8 @@ public: void dpiTraceInit(bool flag) { m_dpiTraceInit = flag; } bool dpiTraceInit() const { return m_dpiTraceInit; } bool isCoroutine() const { return m_rtnType == "VlCoroutine"; } + void recursive(bool flag) { m_recursive = flag; } + bool recursive() const { return m_recursive; } // Special methods bool emptyBody() const { return argsp() == nullptr && initsp() == nullptr && stmtsp() == nullptr diff --git a/src/V3Const.cpp b/src/V3Const.cpp index c2b3e1b1b..d42bb07b0 100644 --- a/src/V3Const.cpp +++ b/src/V3Const.cpp @@ -900,6 +900,7 @@ private: bool m_doNConst = false; // Enable non-constant-child simplifications bool m_doV = false; // Verilog, not C++ conversion bool m_doGenerate = false; // Postpone width checking inside generate + bool m_convertLogicToBit = false; // Convert logical operators to bitwise bool m_hasJumpDelay = false; // JumpGo or Delay under this while bool m_underRecFunc = false; // Under a recursive function AstNodeModule* m_modp = nullptr; // Current module @@ -910,6 +911,7 @@ private: const bool m_globalPass; // ConstVisitor invoked as a global pass static uint32_t s_globalPassNum; // Counts number of times ConstVisitor invoked as global pass V3UniqueNames m_concswapNames; // For generating unique temporary variable names + std::map m_containsMemberAccess; // Caches results of matchBiopToBitwise // METHODS @@ -2322,6 +2324,46 @@ private: iterate(nodep); // Again? } + bool containsMemberAccessRecurse(const AstNode* const nodep) { + if (!nodep) return false; + const auto it = m_containsMemberAccess.lower_bound(nodep); + if (it != m_containsMemberAccess.end() && it->first == nodep) return it->second; + bool result = false; + if (VN_IS(nodep, MemberSel) || VN_IS(nodep, MethodCall) || VN_IS(nodep, CMethodCall)) { + result = true; + } else if (const AstNodeFTaskRef* const funcRefp = VN_CAST(nodep, NodeFTaskRef)) { + if (containsMemberAccessRecurse(funcRefp->taskp())) result = true; + } else if (const AstNodeCCall* const funcRefp = VN_CAST(nodep, NodeCCall)) { + if (containsMemberAccessRecurse(funcRefp->funcp())) result = true; + } else if (const AstNodeFTask* const funcp = VN_CAST(nodep, NodeFTask)) { + // Assume that it has a member access + if (funcp->recursive()) result = true; + } else if (const AstCFunc* const funcp = VN_CAST(nodep, CFunc)) { + if (funcp->recursive()) result = true; + } + if (!result) { + result = containsMemberAccessRecurse(nodep->op1p()) + || containsMemberAccessRecurse(nodep->op2p()) + || containsMemberAccessRecurse(nodep->op3p()) + || containsMemberAccessRecurse(nodep->op4p()); + } + if (!result && !VN_IS(nodep, NodeFTask) + && !VN_IS(nodep, CFunc) // don't enter into next function + && containsMemberAccessRecurse(nodep->nextp())) { + result = true; + } + m_containsMemberAccess.insert(it, std::make_pair(nodep, result)); + return result; + } + + bool matchBiopToBitwise(AstNodeBiop* const nodep) { + if (!m_convertLogicToBit) return false; + if (!nodep->lhsp()->width1()) return false; + if (!nodep->rhsp()->width1()) return false; + if (!nodep->isPure()) return false; + if (containsMemberAccessRecurse(nodep)) return false; + return true; + } bool matchConcatRand(AstConcat* nodep) { // CONCAT(RAND, RAND) - created by Chisel code AstRand* const aRandp = VN_CAST(nodep->lhsp(), Rand); @@ -3598,8 +3640,8 @@ private: TREEOPV("AstOneHot{$lhsp.width1}", "replaceWLhs(nodep)"); TREEOPV("AstOneHot0{$lhsp.width1}", "replaceNum(nodep,1)"); // Binary AND/OR is faster than logical and/or (usually) - TREEOPV("AstLogAnd{$lhsp.width1, $rhsp.width1, nodep->isPure()}", "AstAnd{$lhsp,$rhsp}"); - TREEOPV("AstLogOr {$lhsp.width1, $rhsp.width1, nodep->isPure()}", "AstOr{$lhsp,$rhsp}"); + TREEOPV("AstLogAnd{matchBiopToBitwise(nodep)}", "AstAnd{$lhsp,$rhsp}"); + TREEOPV("AstLogOr {matchBiopToBitwise(nodep)}", "AstOr{$lhsp,$rhsp}"); TREEOPV("AstLogNot{$lhsp.width1}", "AstNot{$lhsp}"); // CONCAT(CONCAT({a},{b}),{c}) -> CONCAT({a},CONCAT({b},{c})) // CONCAT({const},CONCAT({const},{c})) -> CONCAT((constifiedCONC{const|const},{c})) @@ -3718,7 +3760,7 @@ public: case PROC_GENERATE: m_doV = true; m_doNConst = true; m_params = true; m_required = true; m_doGenerate = true; break; case PROC_LIVE: break; - case PROC_V_WARN: m_doV = true; m_doNConst = true; m_warn = true; break; + case PROC_V_WARN: m_doV = true; m_doNConst = true; m_warn = true; m_convertLogicToBit = true; break; case PROC_V_NOWARN: m_doV = true; m_doNConst = true; break; case PROC_V_EXPENSIVE: m_doV = true; m_doNConst = true; m_doExpensive = true; break; case PROC_CPP: m_doV = false; m_doNConst = true; m_doCpp = true; break; diff --git a/src/V3Task.cpp b/src/V3Task.cpp index 5e1ce27a0..993ee99a5 100644 --- a/src/V3Task.cpp +++ b/src/V3Task.cpp @@ -1226,6 +1226,7 @@ private: cfuncp->dpiExportImpl(nodep->dpiExport()); cfuncp->dpiImportWrapper(nodep->dpiImport()); cfuncp->dpiTraceInit(nodep->dpiTraceInit()); + cfuncp->recursive(nodep->recursive()); if (nodep->dpiImport() || nodep->dpiExport()) { cfuncp->isStatic(true); cfuncp->isLoose(true); diff --git a/test_regress/t/t_class_short_circuit.pl b/test_regress/t/t_class_short_circuit.pl new file mode 100755 index 000000000..aabcde63e --- /dev/null +++ b/test_regress/t/t_class_short_circuit.pl @@ -0,0 +1,21 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2020 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); + +compile( + ); + +execute( + check_finished => 1, + ); + +ok(1); +1; diff --git a/test_regress/t/t_class_short_circuit.v b/test_regress/t/t_class_short_circuit.v new file mode 100644 index 000000000..dd7477267 --- /dev/null +++ b/test_regress/t/t_class_short_circuit.v @@ -0,0 +1,33 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2023 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +class Cls; + int x; + function new; + x = 10; + endfunction + function bit set_x(int a); + x = a; + return 1; + endfunction + function int get_x; + return x; + endfunction +endclass + +module t (/*AUTOARG*/); + initial begin + Cls cls; + if (cls != null && cls.x == 10) $stop; + if (cls != null && cls.get_x() == 10) $stop; + cls = new; + if (!cls.set_x(1) || cls.x != 1) $stop; + if (!cls.set_x(2) || cls.get_x() != 2) $stop; + + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_const_opt.pl b/test_regress/t/t_const_opt.pl index 9d9becae5..c46c13c54 100755 --- a/test_regress/t/t_const_opt.pl +++ b/test_regress/t/t_const_opt.pl @@ -20,7 +20,7 @@ execute( ); if ($Self->{vlt}) { - file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 36); + file_grep($Self->{stats}, qr/Optimizations, Const bit op reduction\s+(\d+)/i, 37); } ok(1); 1;