Fix # 0 delays for process resumption, etc. (#4697)

include/verilated_timing.cpp

@@ -57,24 +57,33 @@ void VlDelayScheduler::resume() {
 #ifdef VL_DEBUG
     VL_DEBUG_IF(dump(); VL_DBG_MSGF("         Resuming delayed processes\n"););
 #endif
-    while (awaitingCurrentTime()) {
-        if (m_queue.begin()->first != m_context.time()) {
-            VL_FATAL_MT(__FILE__, __LINE__, "",
-                        "%Error: Encountered process that should've been resumed at an "
-                        "earlier simulation time. Missed a time slot?");
-        }
-        // Remove earliest handle
+    bool resumed = false;
+
+    while (!m_queue.empty() && (m_queue.begin()->first == m_context.time())) {
         VlCoroutineHandle handle = std::move(m_queue.begin()->second);
         m_queue.erase(m_queue.begin());
         handle.resume();
+        resumed = true;
+    }
+
+    if (!m_zeroDelayed.empty()) {
+        for (auto&& handle : m_zeroDelayed) handle.resume();
+        m_zeroDelayed.clear();
+        resumed = true;
+    }
+
+    if (!resumed) {
+        VL_FATAL_MT(__FILE__, __LINE__, "",
+                    "%Error: Encountered process that should've been resumed at an "
+                    "earlier simulation time. Missed a time slot?\n");
     }
 }
 
 uint64_t VlDelayScheduler::nextTimeSlot() const {
-    if (empty()) {
+    if (!m_queue.empty()) return m_queue.begin()->first;
+    if (m_zeroDelayed.empty())
         VL_FATAL_MT(__FILE__, __LINE__, "", "%Error: There is no next time slot scheduled");
-    }
-    return m_queue.begin()->first;
+    return m_context.time();
 }
 
 #ifdef VL_DEBUG
@@ -83,6 +92,12 @@ void VlDelayScheduler::dump() const {
         VL_DBG_MSGF("         No delayed processes:\n");
     } else {
         VL_DBG_MSGF("         Delayed processes:\n");
+        for (auto& susp : m_zeroDelayed) {
+            VL_DBG_MSGF("             Awaiting #0-delayed resumption, "
+                        "time () %" PRIu64 ": ",
+                        m_context.time());
+            susp.dump();
+        }
         for (const auto& susp : m_queue) {
             VL_DBG_MSGF("             Awaiting time %" PRIu64 ": ", susp.first);
             susp.second.dump();

include/verilated_timing.h

@@ -27,6 +27,8 @@
 
 #include "verilated.h"
 
+#include <vector>
+
 // clang-format off
 // Some preprocessor magic to support both Clang and GCC coroutines with both libc++ and libstdc++
 #if defined _LIBCPP_VERSION  // libc++
@@ -148,6 +150,8 @@ public:
 #endif
 };
 
+enum class VlDelayPhase : bool { ACTIVE, INACTIVE };
+
 //=============================================================================
 // VlDelayScheduler stores coroutines to be resumed at a certain simulation time. If the current
 // time is equal to a coroutine's resume time, the coroutine gets resumed.
@@ -155,11 +159,12 @@ public:
 class VlDelayScheduler final {
     // TYPES
     // Time-sorted queue of timestamps and handles
-    using VlDelayedCoroutineQueue = std::multimap<const uint64_t, VlCoroutineHandle>;
+    using VlDelayedCoroutineQueue = std::multimap<uint64_t, VlCoroutineHandle>;
 
     // MEMBERS
     VerilatedContext& m_context;
     VlDelayedCoroutineQueue m_queue;  // Coroutines to be restored at a certain simulation time
+    std::vector<VlCoroutineHandle> m_zeroDelayed;  // Coroutines waiting for #0
 
 public:
     // CONSTRUCTORS
@@ -172,10 +177,11 @@ public:
     // coroutines)
     uint64_t nextTimeSlot() const;
     // Are there no delayed coroutines awaiting?
-    bool empty() const { return m_queue.empty(); }
+    bool empty() const { return m_queue.empty() && m_zeroDelayed.empty(); }
     // Are there coroutines to resume at the current simulation time?
     bool awaitingCurrentTime() const {
-        return !empty() && m_queue.begin()->first <= m_context.time();
+        return (!m_queue.empty() && (m_queue.begin()->first <= m_context.time()))
+               || !m_zeroDelayed.empty();
     }
 #ifdef VL_DEBUG
     void dump() const;
@@ -186,17 +192,34 @@ public:
         struct Awaitable {
             VlProcessRef process;  // Data of the suspended process, null if not needed
             VlDelayedCoroutineQueue& queue;
+            std::vector<VlCoroutineHandle>& queueZeroDelay;
             uint64_t delay;
+            VlDelayPhase phase;
             VlFileLineDebug fileline;
 
             bool await_ready() const { return false; }  // Always suspend
             void await_suspend(std::coroutine_handle<> coro) {
-                queue.emplace(delay, VlCoroutineHandle{coro, process, fileline});
+                if (phase == VlDelayPhase::ACTIVE) {
+                    queue.emplace(delay, VlCoroutineHandle{coro, process, fileline});
+                } else {
+                    queueZeroDelay.emplace_back(VlCoroutineHandle{coro, process, fileline});
+                }
             }
             void await_resume() const {}
         };
-        return Awaitable{process, m_queue, m_context.time() + delay,
-                         VlFileLineDebug{filename, lineno}};
+
+        VlDelayPhase phase = (delay == 0) ? VlDelayPhase::INACTIVE : VlDelayPhase::ACTIVE;
+#ifdef VL_DEBUG
+        if (phase == VlDelayPhase::INACTIVE) {
+            VL_WARN_MT(filename, lineno, VL_UNKNOWN,
+                       "Encountered #0 delay. #0 scheduling support is incomplete and the "
+                       "process will be resumed before combinational logic evaluation.");
+        }
+#endif
+
+        return Awaitable{process,       m_queue,
+                         m_zeroDelayed, m_context.time() + delay,
+                         phase,         VlFileLineDebug{filename, lineno}};
     }
 };
 

src/V3SchedTiming.cpp

@@ -67,11 +67,22 @@ AstCCall* TimingKit::createResume(AstNetlist* const netlistp) {
         m_resumeFuncp->isConst(false);
         m_resumeFuncp->declPrivate(true);
         scopeTopp->addBlocksp(m_resumeFuncp);
+
+        // Put all the timing actives in the resume function
+        AstActive* dlyShedActivep = nullptr;
         for (auto& p : m_lbs) {
-            // Put all the timing actives in the resume function
             AstActive* const activep = p.second;
+            // Hack to ensure that #0 delays will be executed after any other `act` events.
+            // Just handle delayed coroutines last.
+            AstVarRef* const schedrefp = VN_AS(
+                VN_AS(VN_AS(activep->stmtsp(), StmtExpr)->exprp(), CMethodHard)->fromp(), VarRef);
+            if (schedrefp->varScopep()->dtypep()->basicp()->isDelayScheduler()) {
+                dlyShedActivep = activep;
+                continue;
+            }
             m_resumeFuncp->addStmtsp(activep);
         }
+        if (dlyShedActivep) m_resumeFuncp->addStmtsp(dlyShedActivep);
     }
     AstCCall* const callp = new AstCCall{m_resumeFuncp->fileline(), m_resumeFuncp};
     callp->dtypeSetVoid();

src/V3Timing.cpp

@@ -896,10 +896,7 @@ class TimingControlVisitor final : public VNVisitor {
         FileLine* const flp = nodep->fileline();
         AstNodeExpr* valuep = V3Const::constifyEdit(nodep->lhsp()->unlinkFrBack());
         auto* const constp = VN_CAST(valuep, Const);
-        if (constp && constp->isZero()) {
-            nodep->v3warn(ZERODLY, "Unsupported: #0 delays do not schedule process resumption in "
-                                   "the Inactive region");
-        } else {
+        if (!constp || !constp->isZero()) {
             // Scale the delay
             if (valuep->dtypep()->isDouble()) {
                 valuep = new AstRToIRoundS{

test_regress/t/t_delay_var.out

@@ -1,6 +0,0 @@
-%Warning-ZERODLY: t/t_delay_var.v:20:7: Unsupported: #0 delays do not schedule process resumption in the Inactive region
-   20 |       #0 in = 1'b1;
-      |       ^
-                  ... For warning description see https://verilator.org/warn/ZERODLY?v=latest
-                  ... Use "/* verilator lint_off ZERODLY */" and lint_on around source to disable this message.
-%Error: Exiting due to

test_regress/t/t_delay_var.pl

@@ -13,8 +13,6 @@ scenarios(vlt => 1);
 compile(
     verilator_flags2 => ["--exe --main --timing"],
     make_main => 0,
-    fails => $Self->{vlt},
-    expect_filename => $Self->{golden_filename},
     );
 
 execute(

test_regress/t/t_delay_var.v

@@ -17,7 +17,6 @@ module t;
    wire #PDLY d_param = in;
 
    initial begin
-      #0 in = 1'b1;
       #2 in = 1'b0;
       #100;
       $write("*-* All Finished *-*\n");

test_regress/t/t_timing_zerodly.pl

@@ -0,0 +1,23 @@
+#!/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);
+
+compile(
+    verilator_flags2 => ["--exe --main --timing"],
+    make_main => 0,
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_timing_zerodly.v

@@ -0,0 +1,42 @@
+// 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 Foo;
+  static task fork_w_zerodly(longint unsigned current_time);
+    bit my_bit;
+    bit zero_dly_first = 0;
+    // Th code below relies on Verilator's deterministic scheduler and is not
+    // compatible across different simulators.
+    //
+    // The `zero_dly` block is going to be executed first and then suspended at the #0 delay.
+    // Then the `finish_before` block is going to be executed. Once that happens, the
+    // execution of `zero_dly` block should be resumed, all within a single time-slot.
+    //
+    // IF THIS TEST FAILS AFTER CHANGES TO VERILATOR'S SCHEDULER, IT DOESN'T NECESSARILY
+    // MEAN THE CHANGES ARE WRONG.
+    fork
+      begin : zero_dly
+        zero_dly_first = 1;
+        #0;
+        if (current_time != $time) $stop;
+        if (my_bit == 0) $stop;
+      end : zero_dly
+      begin : finish_before
+        if (!zero_dly_first) $stop;
+        my_bit = 1;
+      end : finish_before
+    join_none
+    #1 $display("After fork."); // Check if there's no skipped coroutine
+  endtask
+endclass
+
+module test();
+  initial begin
+    Foo::fork_w_zerodly($time);
+    $write("*-* All Finished *-*\n");
+    $finish;
+  end
+endmodule

test_regress/t/t_timing_zerodly_unsup.out

@@ -1,6 +1,3 @@
-%Warning-ZERODLY: t/t_timing_zerodly_unsup.v:12:13: Unsupported: #0 delays do not schedule process resumption in the Inactive region
-   12 |             #0 if (v) $finish;
-      |             ^
-                  ... For warning description see https://verilator.org/warn/ZERODLY?v=latest
-                  ... Use "/* verilator lint_off ZERODLY */" and lint_on around source to disable this message.
-%Error: Exiting due to
+%Warning: t/t_timing_zerodly_unsup.v:22: Encountered #0 delay. #0 scheduling support is incomplete and the process will be resumed before combinational logic evaluation.
+%Error: t/t_timing_zerodly_unsup.v:23: Verilog $stop
+Aborting...

test_regress/t/t_timing_zerodly_unsup.pl

@@ -8,10 +8,14 @@ if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); di
 # Version 2.0.
 # SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
 
-scenarios(linter => 1);
+scenarios(simulator => 1);
 
-lint(
-    verilator_flags2 => ["--timing"],
+compile(
+    verilator_flags2 => ["--exe --main --timing"],
+    make_main => 0,
+    );
+
+execute(
     fails => 1,
     expect_filename => $Self->{golden_filename},
     );

test_regress/t/t_timing_zerodly_unsup.v

@@ -5,14 +5,23 @@
 // SPDX-License-Identifier: CC0-1.0
 
 module t;
-    event e;
-    logic v = 0;
-    initial #1 begin
-        fork
-            #0 if (v) $finish;
-               else $stop;
-        join_none
-        ->e;
-    end
-    initial @e v = 1;
+  logic v;
+  int num;
+
+  initial begin
+    num = 1;
+    #1;
+    if (v) $stop;
+    num = 21;
+    // Zero delay should postpone the execution and resume it after
+    // evaluating combinational logic which would update `v`. However,
+    // currently we can't postpone the resumption in the current timeframe
+    // past the combinatorial logic evaluation as that is intertwined with
+    // NBA evaluation and partitioned for multithreading. This causes `v`
+    // to not have its value updated despite being checked after #0 delay.
+    #0 if (v) $finish;
+    $stop;
+  end
+
+  always_comb v = (num == 21);
 endmodule