Potential bug in V3Delayed for suspendable/fork

[gezalore]

I'm working on a largeish refactor of V3Delayed and noticed this line:

verilator/src/V3Delayed.cpp

Line 368 in 72b96d5

m_vscpAux(varrefp->varScopep()).suspFinalp = finalp;

And in the larger context:

verilator/src/V3Delayed.cpp

Lines 358 to 373 in 72b96d5

	AstAlwaysPost* finalp = nullptr;
	if (m_inSuspendableOrFork) {
	finalp = m_vscpAux(varrefp->varScopep()).suspFinalp;
	if (!finalp) {
	FileLine* const flp = nodep->fileline();
	finalp = new AstAlwaysPost{flp, nullptr, nullptr};
	UINFO(9, " Created " << finalp << endl);
	if (!m_procp->user3p()) {
	AstActive* const newactp = createActive(varrefp);
	m_procp->user3p(newactp);
	m_vscpAux(varrefp->varScopep()).suspFinalp = finalp;
	}
	AstActive* const actp = VN_AS(m_procp->user3p(), Active);
	actp->addStmtsp(finalp);
	}
	} else {

I'm confused what is happening here. 'finalp' is the "Post" scheduled block that commits the NBA. If this doesn't exist, we create it but only save it if this is the first NBA inside the current process? (If there is a second NBA to a different variable in the same process, we create the 'finalp' but won't save it as m_processp->user3 is already set.) Should line 368 be outside the the if that contains it? Even then the variable might be assigned from multiple suspendable processes with different domains, but only the first ever encountered is considered here? @kbieganski could you shed some light on how this is supposed to work?

FWIW I will not change this in the upcoming patch.

[kbieganski]

I think you're right, this code is completely incorrect.

A quick test that demonstrates it:

module t;
   logic clk1 = 0, clk2 = 0, clk3 = 0, clk4 = 0;
   always #2 clk1 = ~clk1;
   assign #1 clk2 = clk1;
   assign #1 clk3 = clk2;
   assign #1 clk4 = clk3;

   int x = 0;
   int cyc = 0;

   always @(posedge clk1) begin
      if (x != 0) $stop;
      $display("[%0t] clk1 | x=%0d cyc=%0d", $realtime, x, cyc);
      @(posedge clk2);
      $display("[%0t] clk2 | x=%0d cyc=%0d", $realtime, x, cyc);
      x <= x + 1;
      cyc <= cyc + 1;
      if (cyc == 10) $finish;
   end

   always @(posedge clk3) begin
      $display("[%0t] clk3 | x=%0d cyc=%0d", $realtime, x, cyc);
      @(posedge clk4);
      $display("[%0t] clk4 | x=%0d cyc=%0d", $realtime, x, cyc);
      x <= x - 1;
   end
endmodule 

A possible fix:

diff --git i/src/V3Delayed.cpp w/src/V3Delayed.cpp
index cb31f131e..b9837b259 100644
--- i/src/V3Delayed.cpp
+++ w/src/V3Delayed.cpp
@@ -84,8 +84,6 @@ class DelayedVisitor final : public VNVisitor {
         AstVarScope* delayVscp = nullptr;
         // Points to activity block of signal (valid when AstVarScope::user1p is valid)
         AstActive* activep = nullptr;
-        // Post block for this variable used for AssignDlys in suspendable processes
-        AstAlwaysPost* suspFinalp = nullptr;
         // Post block for this variable
         AstAlwaysPost* finalp = nullptr;
         // Last blocking or non-blocking reference
@@ -357,19 +355,15 @@ class DelayedVisitor final : public VNVisitor {
         UINFO(9, "     & " << varrefp << endl);
         AstAlwaysPost* finalp = nullptr;
         if (m_inSuspendableOrFork) {
-            finalp = m_vscpAux(varrefp->varScopep()).suspFinalp;
-            if (!finalp) {
                 FileLine* const flp = nodep->fileline();
                 finalp = new AstAlwaysPost{flp, nullptr, nullptr};
                 UINFO(9, "     Created " << finalp << endl);
                 if (!m_procp->user3p()) {
                     AstActive* const newactp = createActive(varrefp);
                     m_procp->user3p(newactp);
-                    m_vscpAux(varrefp->varScopep()).suspFinalp = finalp;
                 }
                 AstActive* const actp = VN_AS(m_procp->user3p(), Active);
                 actp->addStmtsp(finalp);
-            }
         } else {
             finalp = m_vscpAux(varrefp->varScopep()).finalp;
             if (finalp) {

This generates more AstAlwaysPosts but at least it is (more) correct.

What do you think? Shall I make a PR? Or you can include this in your changes.

[gezalore]

Thanks for checking, I would appreciate a PR (with the test included), after #5092 is merged, which changes a lot of V3Delayed (but that snippet still exists in a largely unmodified form). Maybe we could add some of the unsupported forms, see BLKLOOPINIT warnings after #5092.