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

@@ -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);
 }
 

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}};
     }
 };
 

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<AstNodeExpr*>(dbginfop)->cloneTree(false));
+                }
             } else if (schedulerp->dtypep()->basicp()->isDynamicTriggerScheduler()) {
                 auto* const postp = resumep->cloneTree(false);
                 postp->name("doPostUpdates");

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<AstNode, AstNode>(loopp, stmtsp);
             nodep->replaceWith(loopp);
         }

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

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"],

test_regress/t/t_timing_wait2.out

@@ -0,0 +1,4 @@
+2
+1
+0
+*-* All Finished *-*

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;

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