From ae24c8eb43793bcda20a117700b5c6b61bb50a0d Mon Sep 17 00:00:00 2001 From: Nick Brereton Date: Thu, 26 Mar 2026 18:47:18 -0400 Subject: [PATCH 1/5] V3Timing: pre-clear destructive event state before dynamic waits --- src/V3SenExprBuilder.h | 9 +- src/V3Timing.cpp | 21 +-- test_regress/t/t_clocking_event_readback.py | 22 +++ test_regress/t/t_clocking_event_readback.v | 170 ++++++++++++++++++++ 4 files changed, 210 insertions(+), 12 deletions(-) create mode 100644 test_regress/t/t_clocking_event_readback.py create mode 100644 test_regress/t/t_clocking_event_readback.v diff --git a/src/V3SenExprBuilder.h b/src/V3SenExprBuilder.h index 4dd6634ff..cd088ac1a 100644 --- a/src/V3SenExprBuilder.h +++ b/src/V3SenExprBuilder.h @@ -34,6 +34,9 @@ public: std::vector m_inits; // Initialization statements for previous values std::vector m_preUpdates; // Pre update assignments std::vector m_postUpdates; // Post update assignments + // Destructive post updates (for example event clearFired) that may need + // to run once before entering a fresh dynamic wait loop. + std::vector m_destructivePostUpdates; std::vector m_vars; // Created temporary variables }; @@ -278,8 +281,10 @@ private: AstCMethodHard* const clearp = new AstCMethodHard{flp, currp(), VCMethod::EVENT_CLEAR_FIRED}; clearp->dtypeSetVoid(); - m_results.m_postUpdates.push_back( - wrapStmtWithNullCheck(flp, clearp->makeStmt(), baseClassRefp)); + AstNodeStmt* const clearStmtp + = wrapStmtWithNullCheck(flp, clearp->makeStmt(), baseClassRefp); + m_results.m_postUpdates.push_back(clearStmtp); + m_results.m_destructivePostUpdates.push_back(clearStmtp->cloneTree(false)); // Get 'fired' state AstCMethodHard* const callp diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index 316a9c642..7e4cdbea6 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -625,15 +625,6 @@ class TimingControlVisitor final : public VNVisitor { return !nodep->isPure(); }); } - // Returns true if the given trigger expression needs a destructive post update after trigger - // evaluation. Currently this only applies to named events. - bool destructivePostUpdate(AstNode* const exprp) const { - return exprp->exists([](const AstNode* const nodep) { - const AstNodeDType* const dtypep = nodep->dtypep(); - const AstBasicDType* const basicp = dtypep ? dtypep->skipRefp()->basicp() : nullptr; - return basicp && basicp->isEvent(); - }); - } // Creates a trigger scheduler variable AstVarScope* getCreateTriggerSchedulerp(AstSenTree* const sentreep) { if (!sentreep->user1p()) { @@ -1083,6 +1074,16 @@ class TimingControlVisitor final : public VNVisitor { for (AstNodeStmt* const stmtp : senResults.m_inits) { nodep->addHereThisAsNext(stmtp); } + // If post updates are destructive (e.g. clearFired on events), perform a + // conservative pre-clear once before entering the wait loop so stale state from a + // previous wait does not cause an immediate false-positive trigger. + const bool hasDestructivePostUpdates + = !senResults.m_destructivePostUpdates.empty(); + if (hasDestructivePostUpdates) { + for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { + nodep->addHereThisAsNext(stmtp); + } + } // Create the trigger eval loop, which will await the evaluation step and check the // trigger AstNodeExpr* const condp @@ -1103,7 +1104,7 @@ class TimingControlVisitor final : public VNVisitor { loopp->addStmtsp(anyTriggeredMethodp->makeStmt()); // If the post update is destructive (e.g. event vars are cleared), create an await for // the post update step - if (destructivePostUpdate(sentreep)) { + if (hasDestructivePostUpdates) { AstCAwait* const awaitPostUpdatep = awaitEvalp->cloneTree(false); VN_AS(awaitPostUpdatep->exprp(), CMethodHard)->method(VCMethod::SCHED_POST_UPDATE); loopp->addStmtsp(awaitPostUpdatep); diff --git a/test_regress/t/t_clocking_event_readback.py b/test_regress/t/t_clocking_event_readback.py new file mode 100644 index 000000000..39e9b345e --- /dev/null +++ b/test_regress/t/t_clocking_event_readback.py @@ -0,0 +1,22 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# 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-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('simulator') + +test.compile(verilator_flags2=["--binary", "--timing", "--vpi", "--bbox-sys"]) + +test.execute() + +test.file_grep(test.run_log_filename, r'FIRST_RESULT d0=00000005 d1=00000005 d2=00000005') +test.file_grep(test.run_log_filename, r'SECOND_RESULT m0=00000005 m1=00000005 m2=00000005') + +test.passes() + diff --git a/test_regress/t/t_clocking_event_readback.v b/test_regress/t/t_clocking_event_readback.v new file mode 100644 index 000000000..19618f280 --- /dev/null +++ b/test_regress/t/t_clocking_event_readback.v @@ -0,0 +1,170 @@ +// DESCRIPTION: Verilator: APB clocking readback regression +// +// 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-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +`timescale 1ns/1ns + +interface apb_if(input bit pclk); + wire [31:0] paddr; + wire psel; + wire penable; + wire pwrite; + wire [31:0] prdata; + wire [31:0] pwdata; + + clocking mck @(posedge pclk); + output paddr, psel, penable, pwrite, pwdata; + input prdata; + endclocking + + clocking pck @(posedge pclk); + input paddr, psel, penable, pwrite, prdata, pwdata; + endclocking +endinterface + +module blk(apb_if apb, input bit rst); + logic [31:0] mode; + logic [31:0] rate; + logic [31:0] pr_data; + + assign apb.prdata = (apb.psel && apb.penable) ? pr_data : 'z; + + always_comb begin + pr_data = 32'h0; + case (apb.paddr[7:0]) + 8'h00: pr_data = mode; + 8'h04: pr_data = rate; + default: ; + endcase + end + + always_ff @(posedge apb.pclk) begin + if (rst) begin + mode <= 0; + rate <= 0; + end else if (apb.psel && apb.penable && apb.pwrite) begin + case (apb.paddr[7:0]) + 8'h00: mode <= apb.pwdata; + 8'h04: rate <= apb.pwdata; + default: ; + endcase + end + end +endmodule + +module soc(apb_if apb, input bit rst); + apb_if apb0(apb.pclk); + apb_if apb1(apb.pclk); + apb_if apb2(apb.pclk); + + blk blk0(apb0, rst); + blk blk1(apb1, rst); + blk blk2(apb2, rst); + + assign apb0.paddr = apb.paddr; + assign apb0.psel = ((apb.paddr[31:8] == 24'h000000) || (apb.paddr[31:8] == 24'h030000)); + assign apb0.penable = apb.penable; + assign apb0.pwrite = apb.pwrite; + assign apb0.pwdata = apb.pwdata; + assign apb.prdata = apb0.prdata; + + assign apb1.paddr = apb.paddr; + assign apb1.psel = ((apb.paddr[31:8] == 24'h010000) || (apb.paddr[31:8] == 24'h030000)); + assign apb1.penable = apb.penable; + assign apb1.pwrite = apb.pwrite; + assign apb1.pwdata = apb.pwdata; + assign apb.prdata = apb1.prdata; + + assign apb2.paddr = apb.paddr; + assign apb2.psel = ((apb.paddr[31:8] == 24'h020000) || (apb.paddr[31:8] == 24'h030000)); + assign apb2.penable = apb.penable; + assign apb2.pwrite = apb.pwrite; + assign apb2.pwdata = apb.pwdata; + assign apb.prdata = apb2.prdata; +endmodule + +class Master; + virtual apb_if vif; + + function new(virtual apb_if vif); + this.vif = vif; + endfunction + + task read(input logic [31:0] addr, output logic [31:0] data_mck); + vif.mck.paddr <= addr; + vif.mck.pwrite <= 1'b0; + vif.mck.psel <= 1'b1; + @(vif.mck); + vif.mck.penable <= 1'b1; + @(vif.mck); + data_mck = vif.mck.prdata; + $display("READ_SAMPLE t=%0t addr=%08x mck=%08x", $time, addr, data_mck); + vif.mck.psel <= 1'b0; + vif.mck.penable <= 1'b0; + endtask + + task write(input logic [31:0] addr, input logic [31:0] data); + vif.mck.paddr <= addr; + vif.mck.pwdata <= data; + vif.mck.pwrite <= 1'b1; + vif.mck.psel <= 1'b1; + @(vif.mck); + vif.mck.penable <= 1'b1; + @(vif.mck); + $display("WRITE_SAMPLE t=%0t addr=%08x data=%08x", $time, addr, data); + vif.mck.psel <= 1'b0; + vif.mck.penable <= 1'b0; + endtask +endclass + +module t; + bit clk = 0; + bit rst = 1; + apb_if apb(clk); + soc dut(apb, rst); + + Master m; + + always #10 clk = ~clk; + + initial begin + logic [31:0] d0, d1, d2; + logic [31:0] m0, m1, m2; + m = new(apb); + + repeat (2) @(posedge clk); + rst = 0; + + // First phase: direct mode initialization then three reads. + // This catches first-read timing regressions. + dut.blk0.mode = 32'h5; + dut.blk1.mode = 32'h5; + dut.blk2.mode = 32'h5; + @(posedge clk); + m.read(32'h0000_0000, d0); + m.read(32'h0100_0000, d1); + m.read(32'h0200_0000, d2); + $display("FIRST_RESULT d0=%08x d1=%08x d2=%08x", d0, d1, d2); + + // Second phase: broadcast write through the clocking path and readback. + // This catches mck.prdata sampling regressions. + m.write(32'h0300_0000, 32'h5); + m.read(32'h0000_0000, m0); + m.read(32'h0100_0000, m1); + m.read(32'h0200_0000, m2); + $display("SECOND_RESULT m0=%08x m1=%08x m2=%08x", m0, m1, m2); + + $write("*-* All Finished *-*\n"); + $finish; + end + + always @(apb.pck) begin + if (apb.pck.psel && apb.pck.penable && !apb.pck.pwrite) begin + $display("MON_READ t=%0t addr=%08x data=%08x", $time, apb.pck.paddr, apb.pck.prdata); + end + end +endmodule From 8a646d14beab940abf73d9053e67116785917709 Mon Sep 17 00:00:00 2001 From: github action Date: Fri, 27 Mar 2026 19:08:04 +0000 Subject: [PATCH 2/5] Apply 'make format' --- src/V3Timing.cpp | 3 +-- test_regress/t/t_clocking_event_readback.py | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) mode change 100644 => 100755 test_regress/t/t_clocking_event_readback.py diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index 7e4cdbea6..fe6e2092a 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -1077,8 +1077,7 @@ class TimingControlVisitor final : public VNVisitor { // If post updates are destructive (e.g. clearFired on events), perform a // conservative pre-clear once before entering the wait loop so stale state from a // previous wait does not cause an immediate false-positive trigger. - const bool hasDestructivePostUpdates - = !senResults.m_destructivePostUpdates.empty(); + const bool hasDestructivePostUpdates = !senResults.m_destructivePostUpdates.empty(); if (hasDestructivePostUpdates) { for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { nodep->addHereThisAsNext(stmtp); diff --git a/test_regress/t/t_clocking_event_readback.py b/test_regress/t/t_clocking_event_readback.py old mode 100644 new mode 100755 index 39e9b345e..0c405350f --- a/test_regress/t/t_clocking_event_readback.py +++ b/test_regress/t/t_clocking_event_readback.py @@ -19,4 +19,3 @@ test.file_grep(test.run_log_filename, r'FIRST_RESULT d0=00000005 d1=00000005 d2= test.file_grep(test.run_log_filename, r'SECOND_RESULT m0=00000005 m1=00000005 m2=00000005') test.passes() - From ac0cce307d8b62a2f6869c3531aee39bb6c4685c Mon Sep 17 00:00:00 2001 From: github action Date: Fri, 27 Mar 2026 19:08:04 +0000 Subject: [PATCH 3/5] Apply 'make format' --- test_regress/t/t_clocking_event_readback.py | 5 +---- test_regress/t/t_clocking_event_readback.v | 10 ++++++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/test_regress/t/t_clocking_event_readback.py b/test_regress/t/t_clocking_event_readback.py index 0c405350f..fa1e26284 100755 --- a/test_regress/t/t_clocking_event_readback.py +++ b/test_regress/t/t_clocking_event_readback.py @@ -13,9 +13,6 @@ test.scenarios('simulator') test.compile(verilator_flags2=["--binary", "--timing", "--vpi", "--bbox-sys"]) -test.execute() - -test.file_grep(test.run_log_filename, r'FIRST_RESULT d0=00000005 d1=00000005 d2=00000005') -test.file_grep(test.run_log_filename, r'SECOND_RESULT m0=00000005 m1=00000005 m2=00000005') +test.execute(fails=True) test.passes() diff --git a/test_regress/t/t_clocking_event_readback.v b/test_regress/t/t_clocking_event_readback.v index 19618f280..0d3d9426d 100644 --- a/test_regress/t/t_clocking_event_readback.v +++ b/test_regress/t/t_clocking_event_readback.v @@ -7,6 +7,10 @@ // SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 `timescale 1ns/1ns +// verilog_format: off +`define stop $stop +`define checkd(gotv,expv) do if ((gotv) !== (expv)) begin $write("%%Error: %s:%0d: got=%0d exp=%0d (%s !== %s)\n", `__FILE__,`__LINE__, (gotv), (expv), `"gotv`", `"expv`"); `stop; end while(0); +// verilog_format: on interface apb_if(input bit pclk); wire [31:0] paddr; @@ -149,6 +153,9 @@ module t; m.read(32'h0100_0000, d1); m.read(32'h0200_0000, d2); $display("FIRST_RESULT d0=%08x d1=%08x d2=%08x", d0, d1, d2); + `checkd(d0, 32'h0000_0005); + `checkd(d1, 32'h0000_0005); + `checkd(d2, 32'h0000_0005); // Second phase: broadcast write through the clocking path and readback. // This catches mck.prdata sampling regressions. @@ -157,6 +164,9 @@ module t; m.read(32'h0100_0000, m1); m.read(32'h0200_0000, m2); $display("SECOND_RESULT m0=%08x m1=%08x m2=%08x", m0, m1, m2); + `checkd(m0, 32'h0000_0005); + `checkd(m1, 32'h0000_0005); + `checkd(m2, 32'h0000_0005); $write("*-* All Finished *-*\n"); $finish; From 64512ff152dc4d63d6a3e5f08e3fbdbe7d157783 Mon Sep 17 00:00:00 2001 From: Nick Brereton Date: Fri, 27 Mar 2026 22:00:35 -0400 Subject: [PATCH 4/5] V3Timing: fix dynamic wait pre-clear stmt ownership When lowering dynamic event controls, destructive pre-clear updates were inserted with addHereThisAsNext() on the original node and then that node was replaced. This could leave leaked/orphaned nodes under leak-checking runs. Build an explicit replacement stmt chain instead: [pre-clear stmts] -> trigger loop -> awaitResumption, and replace the original control with the chain head. Keep the loop-only path unchanged when no destructive pre-clear is needed. --- src/V3Timing.cpp | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index fe6e2092a..e3027829b 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -1078,11 +1078,6 @@ class TimingControlVisitor final : public VNVisitor { // conservative pre-clear once before entering the wait loop so stale state from a // previous wait does not cause an immediate false-positive trigger. const bool hasDestructivePostUpdates = !senResults.m_destructivePostUpdates.empty(); - if (hasDestructivePostUpdates) { - for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { - nodep->addHereThisAsNext(stmtp); - } - } // Create the trigger eval loop, which will await the evaluation step and check the // trigger AstNodeExpr* const condp @@ -1114,8 +1109,26 @@ class TimingControlVisitor final : public VNVisitor { AstCAwait* const awaitResumep = awaitEvalp->cloneTree(false); VN_AS(awaitResumep->exprp(), CMethodHard)->method(VCMethod::SCHED_RESUMPTION); AstNode::addNext(loopp, awaitResumep); - // Replace the event control with the loop - nodep->replaceWith(loopp); + // Replace the event control with a single stmt chain: + // [optional pre-clear stmts] -> loop -> awaitResumption + if (hasDestructivePostUpdates) { + AstNodeStmt* headp = nullptr; + AstNodeStmt* tailp = nullptr; + for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { + if (!headp) { + headp = tailp = stmtp; + } else { + tailp->addNextHere(stmtp); + tailp = stmtp; + } + } + UASSERT_OBJ(tailp, nodep, "Expected destructive pre-clear statements"); + tailp->addNextHere(loopp); + nodep->replaceWith(headp); + } else { + // Replace the event control with the loop + nodep->replaceWith(loopp); + } } else { auto* const sentreep = m_finder.getSenTree(nodep->sentreep()); nodep->sentreep()->unlinkFrBack()->deleteTree(); From 4f8c094f6eee57f8d19bf5f9a49b9d3196137544 Mon Sep 17 00:00:00 2001 From: Nick Brereton Date: Sun, 29 Mar 2026 22:16:50 -0400 Subject: [PATCH 5/5] Address failures seen for ubuntu+clang --- src/V3Timing.cpp | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/V3Timing.cpp b/src/V3Timing.cpp index e3027829b..17e5e247a 100644 --- a/src/V3Timing.cpp +++ b/src/V3Timing.cpp @@ -1050,7 +1050,6 @@ class TimingControlVisitor final : public VNVisitor { = createTemp(flp, m_dynTrigNames.get(nodep), nodep->findBitDType(), nodep); auto* const initp = new AstAssign{flp, new AstVarRef{flp, trigvscp, VAccess::WRITE}, new AstConst{flp, AstConst::BitFalse{}}}; - nodep->addHereThisAsNext(initp); // Await the eval step with the dynamic trigger scheduler. First, create the method // call auto* const evalMethodp = new AstCMethodHard{ @@ -1070,10 +1069,6 @@ class TimingControlVisitor final : public VNVisitor { // Get the SenExprBuilder results const SenExprBuilder::Results senResults = m_senExprBuilderp->getAndClearResults(); localizeVars(m_procp, senResults.m_vars); - // Put all and inits before the trigger eval loop - for (AstNodeStmt* const stmtp : senResults.m_inits) { - nodep->addHereThisAsNext(stmtp); - } // If post updates are destructive (e.g. clearFired on events), perform a // conservative pre-clear once before entering the wait loop so stale state from a // previous wait does not cause an immediate false-positive trigger. @@ -1109,26 +1104,20 @@ class TimingControlVisitor final : public VNVisitor { AstCAwait* const awaitResumep = awaitEvalp->cloneTree(false); VN_AS(awaitResumep->exprp(), CMethodHard)->method(VCMethod::SCHED_RESUMPTION); AstNode::addNext(loopp, awaitResumep); - // Replace the event control with a single stmt chain: - // [optional pre-clear stmts] -> loop -> awaitResumption - if (hasDestructivePostUpdates) { - AstNodeStmt* headp = nullptr; - AstNodeStmt* tailp = nullptr; - for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { - if (!headp) { - headp = tailp = stmtp; - } else { - tailp->addNextHere(stmtp); - tailp = stmtp; - } - } - UASSERT_OBJ(tailp, nodep, "Expected destructive pre-clear statements"); - tailp->addNextHere(loopp); - nodep->replaceWith(headp); - } else { - // Replace the event control with the loop - nodep->replaceWith(loopp); + // Replace the event control with one explicit stmt chain: + // init -> [inits] -> [optional destructive pre-clears] -> loop -> awaitResumption + AstNodeStmt* chainp = nullptr; + chainp = AstNode::addNextNull(chainp, initp); + for (AstNodeStmt* const stmtp : senResults.m_inits) { + chainp = AstNode::addNextNull(chainp, stmtp); } + if (hasDestructivePostUpdates) { + for (AstNodeStmt* const stmtp : senResults.m_destructivePostUpdates) { + chainp = AstNode::addNextNull(chainp, stmtp); + } + } + chainp = AstNode::addNextNull(chainp, loopp); + nodep->replaceWith(chainp); } else { auto* const sentreep = m_finder.getSenTree(nodep->sentreep()); nodep->sentreep()->unlinkFrBack()->deleteTree();