From 4b6e15d0eb3ab7f4c1860dc7ba80016b9c4aafa9 Mon Sep 17 00:00:00 2001 From: Krzysztof Bieganski Date: Tue, 6 Dec 2022 13:16:07 +0100 Subject: [PATCH] Fix non-blocking assignments in forks (#3781) (#3800) --- src/V3Delayed.cpp | 20 +++++++++++++++----- test_regress/t/t_timing_bug3781.pl | 28 ++++++++++++++++++++++++++++ test_regress/t/t_timing_bug3781.v | 30 ++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100755 test_regress/t/t_timing_bug3781.pl create mode 100644 test_regress/t/t_timing_bug3781.v diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index ee78c41c5..62835fe89 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -101,6 +101,7 @@ private: bool m_inDly = false; // True in delayed assignments bool m_inLoop = false; // True in for loops bool m_inInitial = false; // True in static initializers and initial blocks + bool m_inSuspendableOrFork = false; // True in suspendable processes and forks bool m_ignoreBlkAndNBlk = false; // Suppress delayed assignment BLKANDNBLK using VarMap = std::map, AstVar*>; VarMap m_modVarMap; // Table of new var names created under module @@ -311,7 +312,7 @@ private: AstVarScope* setvscp; AstAssignPre* setinitp = nullptr; - if (!m_procp->isSuspendable() && nodep->user3p()) { + if (!m_inSuspendableOrFork && nodep->user3p()) { // Simplistic optimization. If the previous statement in same scope was also a =>, // then we told this nodep->user3 we can use its Vdlyvset rather than making a new one. // This is good for code like: @@ -322,7 +323,7 @@ private: const string setvarname = (string("__Vdlyvset__") + oldvarp->shortName() + "__v" + cvtToStr(modVecNum)); setvscp = createVarSc(varrefp->varScopep(), setvarname, 1, nullptr); - if (!m_procp->isSuspendable()) { + if (!m_inSuspendableOrFork) { // Suspendables reset __Vdlyvset__ in the AstAlwaysPost setinitp = new AstAssignPre{ nodep->fileline(), new AstVarRef{nodep->fileline(), setvscp, VAccess::WRITE}, @@ -342,6 +343,8 @@ private: // This ensures that multiple assignments to the same memory will result // in correctly ordered code - the last assignment must be last. // It also has the nice side effect of assisting cache locality. + varrefp->user2Inc(); // Do not generate a warning for the blocking assignment + // that uses this varref AstNodeExpr* selectsp = varrefp; for (int dimension = int(dimreadps.size()) - 1; dimension >= 0; --dimension) { selectsp = new AstArraySel{nodep->fileline(), selectsp, dimreadps[dimension]}; @@ -354,7 +357,7 @@ private: UINFO(9, " For " << setvscp << endl); UINFO(9, " & " << varrefp << endl); AstAlwaysPost* finalp = nullptr; - if (m_procp->isSuspendable()) { + if (m_inSuspendableOrFork) { finalp = VN_AS(varrefp->varScopep()->user3p(), AlwaysPost); if (!finalp) { FileLine* const flp = nodep->fileline(); @@ -400,7 +403,7 @@ private: finalp->user4p(postLogicp); // and the associated IF, as we may be able to reuse it } postLogicp->addThensp(new AstAssign{nodep->fileline(), selectsp, valreadp}); - if (m_procp->isSuspendable()) { + if (m_inSuspendableOrFork) { FileLine* const flp = nodep->fileline(); postLogicp->addThensp(new AstAssign{flp, new AstVarRef{flp, setvscp, VAccess::WRITE}, new AstConst{flp, 0}}); @@ -438,6 +441,8 @@ private: } } void visit(AstNodeProcedure* nodep) override { + VL_RESTORER(m_inSuspendableOrFork); + m_inSuspendableOrFork = nodep->isSuspendable(); m_procp = nodep; m_timingDomains.clear(); iterateChildren(nodep); @@ -464,6 +469,11 @@ private: actp->sensesStorep(clockedDomain); } } + void visit(AstFork* nodep) override { + VL_RESTORER(m_inSuspendableOrFork); + m_inSuspendableOrFork = true; + iterateChildren(nodep); + } void visit(AstCAwait* nodep) override { m_timingDomains.insert(nodep->sensesp()); } void visit(AstFireEvent* nodep) override { UASSERT_OBJ(v3Global.hasEvents(), nodep, "Inconsistent"); @@ -518,7 +528,7 @@ private: const bool isArray = VN_IS(nodep->lhsp(), ArraySel) || (VN_IS(nodep->lhsp(), Sel) && VN_IS(VN_AS(nodep->lhsp(), Sel)->fromp(), ArraySel)); - if (m_procp->isSuspendable() || isArray) { + if (m_inSuspendableOrFork || isArray) { AstNodeExpr* const lhsp = nodep->lhsp(); AstNodeExpr* const newlhsp = createDlyOnSet(nodep, lhsp); if (m_inLoop && isArray) { diff --git a/test_regress/t/t_timing_bug3781.pl b/test_regress/t/t_timing_bug3781.pl new file mode 100755 index 000000000..f86c4b944 --- /dev/null +++ b/test_regress/t/t_timing_bug3781.pl @@ -0,0 +1,28 @@ +#!/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 Antmicro Ltd. 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); + +if (!$Self->have_coroutines) { + skip("No coroutine support"); +} +else { + compile( + verilator_flags2 => ["--exe --main --timing"], + make_main => 0, + ); + + execute( + check_finished => 1, + ); +} + +ok(1); +1; diff --git a/test_regress/t/t_timing_bug3781.v b/test_regress/t/t_timing_bug3781.v new file mode 100644 index 000000000..aca002652 --- /dev/null +++ b/test_regress/t/t_timing_bug3781.v @@ -0,0 +1,30 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2022 by Antmicro Ltd. +// SPDX-License-Identifier: CC0-1.0 + +module t; + logic clk; + logic [7:0] data; + logic [3:0] ptr; + logic [7:0] mem[16]; + + initial begin + clk = 1'b0; + fork forever #5 clk = ~clk; join_none + ptr = '0; + #10 data = 1; + #10 if (mem[ptr] != data) $stop; + #10 data = 2; + #10 if (mem[ptr] != data) $stop; + #10 data = 3; + #10 if (mem[ptr] != data) $stop; + #10 $write("*-* All Finished *-*\n"); + $finish; + end + + always @(posedge clk) begin + mem[ptr] <= #1 data; + end +endmodule