From 5de8ccbf321ff747531def28d4522f27de28bfc5 Mon Sep 17 00:00:00 2001
From: Krzysztof Bieganski <kbieganski@antmicro.com>
Date: Tue, 21 Mar 2023 13:50:53 +0100
Subject: [PATCH] Fix task calls as fork statements (#4055)

Before this patch, calling tasks directly under forks would result in each
statement of these tasks being executed concurrently. This was due to Verilator
inlining tasks most of the time. Such inlined tasks' statements would simply
replace the original call, and there would be no indication that these used to
be grouped together. Ultimately resulting in `V3Timing` treating each statement
as a separate process.

The solution is simply to wrap each fork sub-statement in a begin in `V3Begin`
(except for the ones that are begins, as that would be pointless). `V3Begin` is
already aware of forks, and is supposed to avoid issues like this one, so it
seems like a natural fit. This also protects us from similar bugs, i.e. if some
statement gets replaced or expanded into multiple statements.

Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
---
 src/V3Begin.cpp                          | 13 ++++++++++-
 test_regress/t/t_timing_debug2.out       |  2 +-
 test_regress/t/t_timing_fork_taskcall.pl | 28 ++++++++++++++++++++++++
 test_regress/t/t_timing_fork_taskcall.v  | 23 +++++++++++++++++++
 4 files changed, 64 insertions(+), 2 deletions(-)
 create mode 100755 test_regress/t/t_timing_fork_taskcall.pl
 create mode 100644 test_regress/t/t_timing_fork_taskcall.v

diff --git a/src/V3Begin.cpp b/src/V3Begin.cpp
index b936e33bb..c2a06df4b 100644
--- a/src/V3Begin.cpp
+++ b/src/V3Begin.cpp
@@ -144,9 +144,20 @@ private:
 
     // VISITORS
     void visit(AstFork* nodep) override {
-        // Keep this begin to group its statements together
+        // Keep begins in forks to group their statements together
         VL_RESTORER(m_keepBegins);
         m_keepBegins = true;
+        // If a statement is not a begin, wrap it in a begin. This fixes an issue when the
+        // statement is a task call that gets inlined later (or any other statement that gets
+        // replaced with multiple statements)
+        for (AstNode* stmtp = nodep->stmtsp(); stmtp; stmtp = stmtp->nextp()) {
+            if (!VN_IS(stmtp, Begin)) {
+                AstBegin* const beginp = new AstBegin{stmtp->fileline(), "", nullptr};
+                stmtp->replaceWith(beginp);
+                beginp->addStmtsp(stmtp);
+                stmtp = beginp;
+            }
+        }
         dotNames(nodep, "__FORK__");
         nodep->name("");
     }
diff --git a/test_regress/t/t_timing_debug2.out b/test_regress/t/t_timing_debug2.out
index bbb0b3fe2..3a4ac0ecb 100644
--- a/test_regress/t/t_timing_debug2.out
+++ b/test_regress/t/t_timing_debug2.out
@@ -526,7 +526,7 @@
 -V{t#,#}         Resuming delayed processes
 -V{t#,#}             Resuming: Process waiting at t/t_timing_class.v:91
 -V{t#,#}             Resuming: Process waiting at t/t_timing_class.v:89
--V{t#,#}+      Vt_timing_debug2_t____Vfork_h########__0__1
+-V{t#,#}+      Vt_timing_debug2_t____Vfork_h########__0__0
 -V{t#,#}+        Vt_timing_debug2_t__03a__03aAssignDelayClass::__VnoInFunc_do_assign
 -V{t#,#}+    Vt_timing_debug2___024root___eval_act
 -V{t#,#}+    Vt_timing_debug2___024root___eval_triggers__act
diff --git a/test_regress/t/t_timing_fork_taskcall.pl b/test_regress/t/t_timing_fork_taskcall.pl
new file mode 100755
index 000000000..439181d0a
--- /dev/null
+++ b/test_regress/t/t_timing_fork_taskcall.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 2023 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_fork_taskcall.v b/test_regress/t/t_timing_fork_taskcall.v
new file mode 100644
index 000000000..f2b45b66b
--- /dev/null
+++ b/test_regress/t/t_timing_fork_taskcall.v
@@ -0,0 +1,23 @@
+// 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;
+  task foo;
+    #1 if ($time != 1) $stop;
+    #1 if ($time != 2) $stop;
+    #1 if ($time != 3) $stop;
+  endtask
+
+  initial fork
+    foo;
+    foo;
+    foo;
+    #4 begin
+      $write("*-* All Finished *-*\n");
+      $finish;
+    end
+  join
+endmodule