From 519792d02b470a698b306a6571bf0b1ca7ac8540 Mon Sep 17 00:00:00 2001 From: Krzysztof Bieganski Date: Fri, 26 May 2023 02:20:44 +0200 Subject: [PATCH] Fix to commit coroutines immediately on `wait` statements (#4229) Event-triggered coroutines live in two stages: 'uncommitted' and 'ready'. First they land in 'uncommitted', meaning they can't be resumed yet. Only after coroutines from the 'ready' queue are resumed, the 'uncommitted' ones are moved to the 'ready' queue, and can be resumed. This is to avoid self-triggering in situations like waiting for an event immediately after triggering it. However, there is an issue with `wait` statements. If you have a `wait(b)`, it's being translated into a loop that awaits a change in `b` as long as `b` is false. If `b` is false at first, the coroutine is put into the `uncommitted` queue. If `b` is set to true before it's committed, the coroutine won't get resumed. This patch fixes that by immediately committing event controls created from `wait` statements. That means the coroutine from the example above will get resumed from now on. --- include/verilated_timing.cpp | 5 +-- include/verilated_timing.h | 10 ++++-- src/V3SchedTiming.cpp | 8 ++++- src/V3Timing.cpp | 13 +++++-- test_regress/t/t_lint_wait_bad.out | 8 ++--- test_regress/t/t_lint_wait_bad.pl | 2 +- .../t/{t_timing_wait.pl => t_timing_wait1.pl} | 0 .../t/{t_timing_wait.v => t_timing_wait1.v} | 0 test_regress/t/t_timing_wait2.out | 4 +++ test_regress/t/t_timing_wait2.pl | 29 +++++++++++++++ test_regress/t/t_timing_wait2.v | 35 +++++++++++++++++++ 11 files changed, 100 insertions(+), 14 deletions(-) rename test_regress/t/{t_timing_wait.pl => t_timing_wait1.pl} (100%) rename test_regress/t/{t_timing_wait.v => t_timing_wait1.v} (100%) create mode 100644 test_regress/t/t_timing_wait2.out create mode 100755 test_regress/t/t_timing_wait2.pl create mode 100644 test_regress/t/t_timing_wait2.v diff --git a/include/verilated_timing.cpp b/include/verilated_timing.cpp index f644ed64f..d3fa2cf1b 100644 --- a/include/verilated_timing.cpp +++ b/include/verilated_timing.cpp @@ -95,8 +95,9 @@ void VlTriggerScheduler::resume(const char* eventDescription) { VL_DEBUG_IF(dump(eventDescription); VL_DBG_MSGF(" Resuming processes waiting for %s\n", eventDescription);); #endif - for (auto& susp : m_ready) susp.resume(); - m_ready.clear(); + std::swap(m_ready, m_resumeQueue); + for (VlCoroutineHandle& coro : m_resumeQueue) coro.resume(); + m_resumeQueue.clear(); commit(eventDescription); } diff --git a/include/verilated_timing.h b/include/verilated_timing.h index 019573185..14d570449 100644 --- a/include/verilated_timing.h +++ b/include/verilated_timing.h @@ -207,6 +207,10 @@ class VlTriggerScheduler final { // (not resumable) VlCoroutineVec m_ready; // Coroutines that can be resumed (all coros from m_uncommitted are // moved here in commit()) + VlCoroutineVec m_resumeQueue; // Coroutines being resumed by resume(); kept as a field to + // avoid reallocation. Resumed coroutines are moved to + // m_resumeQueue to allow adding coroutines to m_ready + // during resume(). Outside of resume() should always be empty. public: // METHODS @@ -220,8 +224,8 @@ public: void dump(const char* eventDescription) const; #endif // Used by coroutines for co_awaiting a certain trigger - auto trigger(const char* eventDescription = VL_UNKNOWN, const char* filename = VL_UNKNOWN, - int lineno = 0) { + auto trigger(bool commit, const char* eventDescription = VL_UNKNOWN, + const char* filename = VL_UNKNOWN, int lineno = 0) { VL_DEBUG_IF(VL_DBG_MSGF(" Suspending process waiting for %s at %s:%d\n", eventDescription, filename, lineno);); struct Awaitable { @@ -234,7 +238,7 @@ public: } void await_resume() const {} }; - return Awaitable{m_uncommitted, VlFileLineDebug{filename, lineno}}; + return Awaitable{commit ? m_ready : m_uncommitted, VlFileLineDebug{filename, lineno}}; } }; diff --git a/src/V3SchedTiming.cpp b/src/V3SchedTiming.cpp index cc5eba6b9..e0d3e908d 100644 --- a/src/V3SchedTiming.cpp +++ b/src/V3SchedTiming.cpp @@ -166,7 +166,13 @@ TimingKit prepareTiming(AstNetlist* const netlistp) { flp, new AstVarRef{flp, schedulerp, VAccess::READWRITE}, "resume"}; resumep->dtypeSetVoid(); if (schedulerp->dtypep()->basicp()->isTriggerScheduler()) { - if (methodp->pinsp()) resumep->addPinsp(methodp->pinsp()->cloneTree(false)); + UASSERT_OBJ(methodp->pinsp(), methodp, + "Trigger method should have pins from V3Timing"); + // The first pin is the commit boolean, the rest (if any) should be debug info + // See V3Timing for details + if (AstNode* const dbginfop = methodp->pinsp()->nextp()) { + resumep->addPinsp(static_cast(dbginfop)->cloneTree(false)); + } } else if (schedulerp->dtypep()->basicp()->isDynamicTriggerScheduler()) { auto* const postp = resumep->cloneTree(false); postp->name("doPostUpdates"); diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index 184ba25d8..8f4cf323c 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -237,6 +237,9 @@ private: // to this sentree // Ast{NodeProcedure,CFunc,Begin}::user2() -> bool. Set true if process/task // is suspendable + // Ast{EventControl}::user2() -> bool. Set true if event control + // should immediately be + // committed // AstSenTree::user2() -> AstCExpr*. Debug info passed to the // timing schedulers // const VNUser1InUse m_user1InUse; (Allocated for use in SuspendableVisitor) @@ -709,6 +712,9 @@ private: flp, new AstVarRef{flp, getCreateTriggerSchedulerp(sensesp), VAccess::WRITE}, "trigger"}; triggerMethodp->dtypeSetVoid(); + // If it should be committed immediately, pass true, otherwise false + triggerMethodp->addPinsp(nodep->user2() ? new AstConst{flp, AstConst::BitTrue{}} + : new AstConst{flp, AstConst::BitFalse{}}); addEventDebugInfo(triggerMethodp, sensesp); // Create the co_await AstCAwait* const awaitp = new AstCAwait{flp, triggerMethodp, sensesp}; @@ -835,9 +841,10 @@ private: AstSenItem* const senItemsp = varRefpsToSenItemsp(condp); UASSERT_OBJ(senItemsp, nodep, "No varrefs in wait statement condition"); // Put the event control in a while loop with the wait expression as condition - auto* const loopp - = new AstWhile{flp, new AstLogNot{flp, condp}, - new AstEventControl{flp, new AstSenTree{flp, senItemsp}, nullptr}}; + AstEventControl* const controlp + = new AstEventControl{flp, new AstSenTree{flp, senItemsp}, nullptr}; + controlp->user2(true); // Commit immediately + AstWhile* const loopp = new AstWhile{flp, new AstLogNot{flp, condp}, controlp}; if (stmtsp) AstNode::addNext(loopp, stmtsp); nodep->replaceWith(loopp); } diff --git a/test_regress/t/t_lint_wait_bad.out b/test_regress/t/t_lint_wait_bad.out index 3b06a1c50..908d043be 100644 --- a/test_regress/t/t_lint_wait_bad.out +++ b/test_regress/t/t_lint_wait_bad.out @@ -1,15 +1,15 @@ -%Warning-WAITCONST: t/t_timing_wait.v:48:12: Wait statement condition is constant +%Warning-WAITCONST: t/t_timing_wait1.v:48:12: Wait statement condition is constant 48 | wait(1); | ^ ... For warning description see https://verilator.org/warn/WAITCONST?v=latest ... Use "/* verilator lint_off WAITCONST */" and lint_on around source to disable this message. -%Warning-WAITCONST: t/t_timing_wait.v:50:14: Wait statement condition is constant +%Warning-WAITCONST: t/t_timing_wait1.v:50:14: Wait statement condition is constant 50 | wait(0 < 1) $write("*-* All Finished *-*\n"); | ^ -%Warning-WAITCONST: t/t_timing_wait.v:54:17: Wait statement condition is constant +%Warning-WAITCONST: t/t_timing_wait1.v:54:17: Wait statement condition is constant 54 | initial wait(0) $stop; | ^ -%Warning-WAITCONST: t/t_timing_wait.v:55:19: Wait statement condition is constant +%Warning-WAITCONST: t/t_timing_wait1.v:55:19: Wait statement condition is constant 55 | initial wait(1 == 0) $stop; | ^~ %Error: Exiting due to diff --git a/test_regress/t/t_lint_wait_bad.pl b/test_regress/t/t_lint_wait_bad.pl index 7500d4750..b92ab79f3 100755 --- a/test_regress/t/t_lint_wait_bad.pl +++ b/test_regress/t/t_lint_wait_bad.pl @@ -10,7 +10,7 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di scenarios(vlt => 1); -top_filename("t/t_timing_wait.v"); +top_filename("t/t_timing_wait1.v"); lint( verilator_flags2 => ["--timing"], diff --git a/test_regress/t/t_timing_wait.pl b/test_regress/t/t_timing_wait1.pl similarity index 100% rename from test_regress/t/t_timing_wait.pl rename to test_regress/t/t_timing_wait1.pl diff --git a/test_regress/t/t_timing_wait.v b/test_regress/t/t_timing_wait1.v similarity index 100% rename from test_regress/t/t_timing_wait.v rename to test_regress/t/t_timing_wait1.v diff --git a/test_regress/t/t_timing_wait2.out b/test_regress/t/t_timing_wait2.out new file mode 100644 index 000000000..3916a7dbf --- /dev/null +++ b/test_regress/t/t_timing_wait2.out @@ -0,0 +1,4 @@ +2 +1 +0 +*-* All Finished *-* diff --git a/test_regress/t/t_timing_wait2.pl b/test_regress/t/t_timing_wait2.pl new file mode 100755 index 000000000..3860b2943 --- /dev/null +++ b/test_regress/t/t_timing_wait2.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 2023 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); + +if (!$Self->have_coroutines) { + skip("No coroutine support"); +} +else { + compile( + verilator_flags2 => ["--exe --timing --main"], + make_main => 0, + ); + + execute( + check_finished => 1, + expect_filename => $Self->{golden_filename}, + ); +} + +ok(1); +1; diff --git a/test_regress/t/t_timing_wait2.v b/test_regress/t/t_timing_wait2.v new file mode 100644 index 000000000..0bb5ec6b5 --- /dev/null +++ b/test_regress/t/t_timing_wait2.v @@ -0,0 +1,35 @@ +// 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 + +module t; + bit s[3:0] = {0, 0, 0, 0}; + + initial begin + wait (s[1]); + s[0] = 1; + $display("0"); + end + + initial begin + wait (s[2]); + s[1] = 1; + $display("1"); + #1 $write("*-* All Finished *-*\n"); + $finish; + end + + initial begin + wait (s[3]); + s[2] = 1; + $display("2"); + end + + initial begin + s[3] = 1; + end + + initial #2 $stop; // timeout +endmodule