Fix inlining of forks (#3594)

Before this change, some forked processes were being inlined in `V3Timing` because they contained no `CAwait`s. This only works under the assumption that no `CAwait`s will be added there later, which is not true, as a function called by a forked process could be turned into a coroutine later. The call would be wrapped in a new `CAwait`, but the process itself would have already been inlined at this point. This commit moves the inlining to `transformForks` in `V3SchedTiming`, which is called at a point when all `CAwait`s are already in place. Signed-off-by: Krzysztof Bieganski <kbieganski@antmicro.com>
2025-04-16 01:26:54 +00:00 · 2022-09-05 16:19:19 +02:00 · 2022-09-05 16:19:19 +02:00 · a2e1b32a1c
commit a2e1b32a1c
parent 54f89bce42
4 changed files with 54 additions and 45 deletions
--- a/src/V3SchedTiming.cpp
+++ b/src/V3SchedTiming.cpp
@ -242,6 +242,7 @@ void transformForks(AstNetlist* const netlistp) {

        // STATE
        bool m_inClass = false;  // Are we in a class?
+        bool m_beginHasAwaits = false;  // Does the current begin have awaits?
        AstFork* m_forkp = nullptr;  // Current fork
        AstCFunc* m_funcp = nullptr;  // Current function

@ -309,8 +310,11 @@ void transformForks(AstNetlist* const netlistp) {
            iterateChildren(nodep);
            m_funcp = nullptr;
        }
-        virtual void visit(AstVar* nodep) override { nodep->user1(true); }
+        virtual void visit(AstVar* nodep) override {
+            if (!m_forkp) nodep->user1(true);
+        }
        virtual void visit(AstFork* nodep) override {
+            if (m_forkp) return;  // Handle forks in forks after moving them to new functions
            VL_RESTORER(m_forkp);
            m_forkp = nodep;
            iterateChildrenConst(nodep);  // Const, so we don't iterate the calls twice
@ -321,28 +325,40 @@ void transformForks(AstNetlist* const netlistp) {
        }
        virtual void visit(AstBegin* nodep) override {
            UASSERT_OBJ(m_forkp, nodep, "Begin outside of a fork");
-            UASSERT_OBJ(!nodep->name().empty(), nodep, "Begin needs a name");
-            FileLine* const flp = nodep->fileline();
-            // Create a function to put this begin's statements in
-            AstCFunc* const newfuncp
-                = new AstCFunc{flp, nodep->name(), m_funcp->scopep(), "VlCoroutine"};
-            m_funcp->addNextHere(newfuncp);
-            newfuncp->isLoose(m_funcp->isLoose());
-            newfuncp->slow(m_funcp->slow());
-            newfuncp->isConst(m_funcp->isConst());
-            newfuncp->declPrivate(true);
-            // Replace the begin with a call to the newly created function
-            auto* const callp = new AstCCall{flp, newfuncp};
-            nodep->replaceWith(callp);
-            // If we're in a class, add a vlSymsp arg
-            if (m_inClass) {
-                newfuncp->argTypes(EmitCBaseVisitor::symClassVar());
-                callp->argTypes("vlSymsp");
+            // Start with children, so later we only find awaits that are actually in this begin
+            m_beginHasAwaits = false;
+            iterateChildrenConst(nodep);
+            if (m_beginHasAwaits) {
+                UASSERT_OBJ(!nodep->name().empty(), nodep, "Begin needs a name");
+                // Create a function to put this begin's statements in
+                FileLine* const flp = nodep->fileline();
+                AstCFunc* const newfuncp
+                    = new AstCFunc{flp, nodep->name(), m_funcp->scopep(), "VlCoroutine"};
+                m_funcp->addNextHere(newfuncp);
+                newfuncp->isLoose(m_funcp->isLoose());
+                newfuncp->slow(m_funcp->slow());
+                newfuncp->isConst(m_funcp->isConst());
+                newfuncp->declPrivate(true);
+                // Replace the begin with a call to the newly created function
+                auto* const callp = new AstCCall{flp, newfuncp};
+                nodep->replaceWith(callp);
+                // If we're in a class, add a vlSymsp arg
+                if (m_inClass) {
+                    newfuncp->argTypes(EmitCBaseVisitor::symClassVar());
+                    callp->argTypes("vlSymsp");
+                }
+                // Put the begin's statements in the function, delete the begin
+                newfuncp->addStmtsp(nodep->stmtsp()->unlinkFrBackWithNext());
+                remapLocals(newfuncp, callp);
+            } else {
+                // No awaits, just inline the forked process
+                nodep->replaceWith(nodep->stmtsp()->unlinkFrBackWithNext());
            }
-            // Put the begin's statements in the function, delete the begin
-            newfuncp->addStmtsp(nodep->stmtsp()->unlinkFrBackWithNext());
            VL_DO_DANGLING(nodep->deleteTree(), nodep);
-            remapLocals(newfuncp, callp);
+        }
+        virtual void visit(AstCAwait* nodep) override {
+            m_beginHasAwaits = true;
+            iterateChildrenConst(nodep);
        }

        //--------------------
--- a/src/V3Timing.cpp
+++ b/src/V3Timing.cpp
@ -614,25 +614,12 @@ private:
            VL_RESTORER(m_procp);
            m_procp = beginp;
            iterate(beginp);
-            if (!m_procp->user2()) {
-                // No awaits, we can inline this process
-                if (auto* const stmtsp = beginp->stmtsp()) {
-                    nodep->addHereThisAsNext(stmtsp->unlinkFrBackWithNext());
-                }
-                VL_DO_DANGLING(beginp->unlinkFrBack()->deleteTree(), beginp);
-                // We inlined at least one process, so we can consider it joined; convert join_any
-                // to join_none
-                if (nodep->joinType().joinAny()) nodep->joinType(VJoinType::JOIN_NONE);
-            } else {
-                // Name the begin (later the name will be used for a new function)
-                beginp->name(nodep->name() + "__" + cvtToStr(idx++));
-            }
-        }
-        if (!nodep->stmtsp()) {
-            VL_DO_DANGLING(nodep->unlinkFrBack()->deleteTree(), nodep);
-        } else if (!nodep->joinType().joinNone()) {
-            makeForkJoin(nodep);
+            // Even if we do not find any awaits, we cannot simply inline the process here, as new
+            // awaits could be added later.
+            // Name the begin (later the name will be used for a new function)
+            beginp->name(nodep->name() + "__" + cvtToStr(idx++));
        }
+        if (!nodep->joinType().joinNone()) makeForkJoin(nodep);
    }

    //--------------------
--- a/test_regress/t/t_timing_debug2.out
+++ b/test_regress/t/t_timing_debug2.out
@ -6,11 +6,13 @@
 -V{t#,#}+    Vt_timing_debug2___024root___eval_static
 -V{t#,#}+    Vt_timing_debug2___024root___eval_initial
 -V{t#,#}+    Vt_timing_debug2___024root___eval_initial__TOP__0
-[0] fork..join process 4
-V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__0
+-V{t#,#}             Process forked at t/t_timing_fork_join.v:14 finished
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__1
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__2
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__3
+[0] fork..join process 4
+-V{t#,#}             Process forked at t/t_timing_fork_join.v:18 finished
+-V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__5
 -V{t#,#}             Awaiting join of fork at: t/t_timing_fork_join.v:13
 -V{t#,#}+    Vt_timing_debug2___024root___eval_initial__TOP__1
 -V{t#,#}+    Vt_timing_debug2___024root___eval_settle
@ -127,10 +129,11 @@
 -V{t#,#}         Resuming delayed processes
 -V{t#,#}             Resuming: Process waiting at t/t_timing_fork_join.v:16
 [16] fork in fork starts
-[16] fork..join process 8
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__0
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__1
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__2
+[16] fork..join process 8
+-V{t#,#}             Process forked at t/t_timing_fork_join.v:25 finished
 -V{t#,#}             Awaiting join of fork at: t/t_timing_fork_join.v:21
 -V{t#,#}+    Vt_timing_debug2___024root___eval_act
 -V{t#,#}+    Vt_timing_debug2___024root___eval_triggers__act
@ -242,9 +245,11 @@
 -V{t#,#}         Resuming delayed processes
 -V{t#,#}             Resuming: Process waiting at t/t_timing_fork_join.v:30
 [64] main process
-fork..join_any process 2
 -V{t#,#}+    Vt_timing_debug2___024root____Vfork___h########__0__0
 -V{t#,#}         Suspending process waiting for @([event] t.e1) at t/t_timing_fork_join.v:33
+fork..join_any process 2
+-V{t#,#}             Process forked at t/t_timing_fork_join.v:37 finished
+-V{t#,#}             Awaiting join of fork at: t/t_timing_fork_join.v:31
 back in main process
 -V{t#,#}+    Vt_timing_debug2___024root___eval_act
 -V{t#,#}+    Vt_timing_debug2___024root___eval_triggers__act
@ -283,6 +288,7 @@ back in main process
 -V{t#,#}         Resuming processes waiting for @([event] t.e1)
 -V{t#,#}             Resuming: Process waiting at t/t_timing_fork_join.v:33
 fork..join_any process 1
+-V{t#,#}             Process forked at t/t_timing_fork_join.v:32 finished
 -V{t#,#}+    Vt_timing_debug2___024root___eval_act
 -V{t#,#}+    Vt_timing_debug2___024root___eval_triggers__act
 -V{t#,#}+    Vt_timing_debug2___024root___dump_triggers__act
--- a/test_regress/t/t_timing_fork_comb.v
+++ b/test_regress/t/t_timing_fork_comb.v
@ -48,9 +48,9 @@ module t;
       if (a != 10) $stop;
       if (b != 20) $stop;
       if (c != 30) $stop;
-       if (d != 50) $stop;
+       if (d != 45) $stop;
       if (e != 75) $stop;
-       if (f != 125) $stop;
+       if (f != 107) $stop;
       if (v != 'b001010) $stop;
       $write("*-* All Finished *-*\n");
       $finish;