diff --git a/src/V3AssertPre.cpp b/src/V3AssertPre.cpp index baedef97f..8fe9fb5c5 100644 --- a/src/V3AssertPre.cpp +++ b/src/V3AssertPre.cpp @@ -40,10 +40,18 @@ class AssertPreVisitor final : public VNVisitor { // Eventually inlines calls to sequences, properties, etc. // We're not parsing the tree, or anything more complicated. private: + // TYPES + struct SynchDrive final { + AstClockingItem* itemp; // Clocking item of the driven clockvar + AstNodeExpr* refp; // Reference to the driven clockvar + }; + // NODE STATE // AstClockingItem::user1p() // AstVar*. varp() of ClockingItem after unlink + // AstClockingItem::user2p() // AstVar*. Flag set by drives of output clockvar // AstPExpr::user1() // bool. Created from AstUntil const VNUser1InUse m_inuser1; + const VNUser2InUse m_inuser2; // STATE // Current context: AstNetlist* const m_netlistp = nullptr; // Current netlist @@ -71,15 +79,48 @@ private: V3UniqueNames m_blockNames{"__VassertBlock"}; // Names of blocks with temporaries V3UniqueNames m_propVarNames{"__Vpropvar"}; // Property-local variable name generator V3UniqueNames m_activeNames{"__VassertsActive"}; // Active asserts map name generator + V3UniqueNames m_drivenNames{"__VclockingDriven"}; // Clockvar drive flag name generator bool m_inAssign = false; // True if in an AssignNode bool m_inAssignDlyLhs = false; // True if in AssignDly's LHS bool m_inSynchDrive = false; // True if in synchronous drive + std::vector m_drives; // Clockvars written by the synchronous drive bool m_hasCycleDelay = false; // True if node has cycle delay beneath std::vector m_xrefsp; // list of xrefs that need name fixup std::vector m_seqsToCleanup; // Sequences to clean up after traversal // METHODS + // Flag set by each drive of an output clockvar, added to the module with its clocking item + AstVar* getCreateDrivenVarp(AstClockingItem* itemp) { + if (!itemp->user2p()) { + AstVar* const varp = new AstVar{itemp->fileline(), VVarType::MODULETEMP, + m_drivenNames.get(itemp), itemp->findBitDType()}; + varp->lifetime(VLifetime::STATIC_EXPLICIT); + itemp->user2p(varp); + } + return VN_AS(itemp->user2p(), Var); + } + // Assignment setting the drive flag of a driven clockvar, referenced like the clockvar + AstAssign* newDrivenSetp(FileLine* flp, const SynchDrive& drive) { + AstVar* const varp = getCreateDrivenVarp(drive.itemp); + AstNodeExpr* refp; + if (const AstVarXRef* const xrefp = VN_CAST(drive.refp, VarXRef)) { + refp = new AstVarXRef{flp, varp, xrefp->dotted(), VAccess::WRITE}; + } else if (const AstMemberSel* const selp = VN_CAST(drive.refp, MemberSel)) { + // The interface expression is evaluated again for the flag, so it must not have side + // effects; cloneTreePure warns if it has any + AstMemberSel* const newSelp + = new AstMemberSel{flp, selp->fromp()->cloneTreePure(false), varp}; + newSelp->access(VAccess::WRITE); + refp = newSelp; + } else { + refp = new AstVarRef{flp, varp, VAccess::WRITE}; + } + AstAssign* const setp = new AstAssign{flp, refp, new AstConst{flp, AstConst::BitTrue{}}}; + setp->user1(true); + return setp; + } + static void checkSamplingFuncDType(AstNodeExpr* nodep, const AstNode* exprp) { const AstNodeDType* const dtypep = exprp->dtypep()->skipRefp(); if (!dtypep->isIntegralOrPacked()) { @@ -273,13 +314,21 @@ private: // It has to be converted to a list of ModportClockingVarRefs, // because clocking blocks are removed in this pass for (AstNode* itemp = nodep->clockingp()->itemsp(); itemp; itemp = itemp->nextp()) { - if (const AstClockingItem* citemp = VN_CAST(itemp, ClockingItem)) { + if (AstClockingItem* const citemp = VN_CAST(itemp, ClockingItem)) { if (AstVar* const varp = citemp->varp() ? citemp->varp() : VN_AS(citemp->user1p(), Var)) { AstModportVarRef* const modVarp = new AstModportVarRef{ nodep->fileline(), varp->name(), citemp->direction()}; modVarp->varp(varp); nodep->addNextHere(modVarp); + if (citemp->direction().isOutput()) { + // Drives through the modport set the drive flag + AstVar* const drivenVarp = getCreateDrivenVarp(citemp); + AstModportVarRef* const drivenRefp = new AstModportVarRef{ + nodep->fileline(), drivenVarp->name(), VDirection::OUTPUT}; + drivenRefp->varp(drivenVarp); + nodep->addNextHere(drivenRefp); + } } } } @@ -292,6 +341,8 @@ private: // Unused item return; } + // Flag set by drives of an output clockvar, possibly visited before this item + if (nodep->direction().isOutput()) m_modp->addStmtsp(getCreateDrivenVarp(nodep)); FileLine* const flp = nodep->fileline(); V3Const::constifyEdit(nodep->skewp()); if (!VN_IS(nodep->skewp(), Const)) { @@ -318,32 +369,26 @@ private: AstInitialStatic* const initClockvarp = new AstInitialStatic{ flp, new AstAssign{flp, skewedWriteRefp, exprp->cloneTreePure(false)}}; m_modp->addStmtsp(initClockvarp); - // A var to keep the previous value of the clockvar - AstVar* const prevVarp = new AstVar{ - flp, VVarType::MODULETEMP, "__Vclocking_prev__" + varp->name(), exprp->dtypep()}; - prevVarp->lifetime(VLifetime::STATIC_EXPLICIT); - AstInitialStatic* const initPrevClockvarp = new AstInitialStatic{ - flp, new AstAssign{flp, new AstVarRef{flp, prevVarp, VAccess::WRITE}, - skewedReadRefp->cloneTreePure(false)}}; - m_modp->addStmtsp(prevVarp); - m_modp->addStmtsp(initPrevClockvarp); - // Assign the clockvar to the actual var; only do it if the clockvar's value has - // changed + // Each drive sets a flag, so the signal is also assigned when driven with the same + // value again, e.g. after another assignment to the signal (IEEE 1800-2023 14.16.2) + AstVar* const drivenVarp = getCreateDrivenVarp(nodep); + m_modp->addStmtsp(new AstInitialStatic{ + flp, new AstAssign{flp, new AstVarRef{flp, drivenVarp, VAccess::WRITE}, + new AstConst{flp, AstConst::BitFalse{}}}}); + // Assign the clockvar to the actual var if it was driven AstAssign* const assignp = new AstAssign{flp, exprp->cloneTreePure(false), skewedReadRefp}; AstIf* const ifp - = new AstIf{flp, - new AstNeq{flp, new AstVarRef{flp, prevVarp, VAccess::READ}, - skewedReadRefp->cloneTreePure(false)}, - assignp}; - ifp->addThensp(new AstAssign{flp, new AstVarRef{flp, prevVarp, VAccess::WRITE}, - skewedReadRefp->cloneTree(false)}); + = new AstIf{flp, new AstVarRef{flp, drivenVarp, VAccess::READ}, + new AstAssign{flp, new AstVarRef{flp, drivenVarp, VAccess::WRITE}, + new AstConst{flp, AstConst::BitFalse{}}}}; + ifp->addThensp(assignp); if (skewp->isZero()) { // Drive the var in Re-NBA (IEEE 1800-2023 14.16) AstSenTree* senTreep = new AstSenTree{flp, m_clockingp->sensesp()->cloneTree(false)}; - senTreep->addSensesp( - new AstSenItem{flp, VEdgeType::ET_CHANGED, skewedReadRefp->cloneTree(false)}); + senTreep->addSensesp(new AstSenItem{ + flp, VEdgeType::ET_CHANGED, new AstVarRef{flp, drivenVarp, VAccess::READ}}); AstCMethodHard* const trigp = new AstCMethodHard{ nodep->fileline(), new AstVarRef{flp, m_clockingp->ensureEventp(), VAccess::READ}, @@ -620,7 +665,10 @@ private: nodep->v3error("Only non-blocking assignments can write " "to clockvars (IEEE 1800-2023 14.16)"); } - if (m_inAssign) m_inSynchDrive = true; + if (m_inAssign) { + m_inSynchDrive = true; + m_drives.push_back({itemp, nodep}); + } } else if (itemp->direction() == VDirection::INPUT) { nodep->v3error("Cannot write to input clockvar (IEEE 1800-2023 14.3)"); } @@ -641,7 +689,10 @@ private: nodep->v3error("Only non-blocking assignments can write " "to clockvars (IEEE 1800-2023 14.16)"); } - if (m_inAssign) m_inSynchDrive = true; + if (m_inAssign) { + m_inSynchDrive = true; + m_drives.push_back({itemp, nodep}); + } } else if (itemp->direction() == VDirection::INPUT) { nodep->v3error("Cannot write to input clockvar (IEEE 1800-2023 14.3)"); } @@ -652,6 +703,7 @@ private: if (nodep->user1()) return; VL_RESTORER(m_inAssign); VL_RESTORER(m_inSynchDrive); + VL_RESTORER_CLEAR(m_drives); m_inAssign = true; m_inSynchDrive = false; { @@ -668,6 +720,20 @@ private: assignp->user1(true); nodep->replaceWith(assignp); VL_DO_DANGLING(nodep->deleteTree(), nodep); + nodep = assignp; + } + // Note the drives when they take effect, after the cycle delay if any. Each clockvar + // of the LHS is driven, e.g. in a concatenation which other simulators accept, although + // IEEE 1800-2023 14.16 does not allow it + AstNode* setsp = nullptr; + for (const SynchDrive& drive : m_drives) { + setsp = AstNode::addNext(setsp, newDrivenSetp(nodep->fileline(), drive)); + } + if (!setsp) return; + if (AstBegin* const beginp = VN_CAST(nodep->timingControlp(), Begin)) { + beginp->addStmtsp(setsp); + } else { + nodep->addNextHere(setsp); } } void visit(AstAlways* nodep) override { diff --git a/src/V3AstAttr.h b/src/V3AstAttr.h index efe5bb6de..5cbb38956 100644 --- a/src/V3AstAttr.h +++ b/src/V3AstAttr.h @@ -1364,6 +1364,7 @@ public: bool isNonOutput() const { return m_e == INPUT || m_e == INOUT || m_e == REF || m_e == CONSTREF; } + bool isOutput() const { return m_e == OUTPUT; } bool isReadOnly() const VL_MT_SAFE { return m_e == INPUT || m_e == CONSTREF; } bool isWritable() const VL_MT_SAFE { return m_e == OUTPUT || m_e == INOUT || m_e == REF; } bool isRef() const VL_MT_SAFE { return m_e == REF; } diff --git a/test_regress/t/t_clocking_drive_concat.py b/test_regress/t/t_clocking_drive_concat.py new file mode 100755 index 000000000..dc82c21fc --- /dev/null +++ b/test_regress/t/t_clocking_drive_concat.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Synchronous drive of a concatenation of clockvars +# +# 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"]) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_clocking_drive_concat.v b/test_regress/t/t_clocking_drive_concat.v new file mode 100644 index 000000000..df859429c --- /dev/null +++ b/test_regress/t/t_clocking_drive_concat.v @@ -0,0 +1,47 @@ +// DESCRIPTION: Verilator: Synchronous drive of a concatenation of clockvars +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// verilog_format: off +`define stop $stop +`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got=\"%s\" exp=\"%s\"\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +module t; + bit clk; + bit a; + bit b; + string a_log; + string b_log; + + always #5 clk = ~clk; + + clocking pe @(posedge clk); + output a, b; + endclocking + + always @(a) if ($time != 0) a_log = {a_log, $sformatf("%0d@%0d ", a, $time)}; + always @(b) if ($time != 0) b_log = {b_log, $sformatf("%0d@%0d ", b, $time)}; + + // IEEE 1800-2023 14.16 does not allow a concatenation as the target of a synchronous drive, + // but other simulators accept it and drive each clockvar, also with the value last driven + initial begin + @(pe); + {pe.a, pe.b} <= 2'b11; + #2; + a = 0; + b = 0; + @(pe); + {pe.a, pe.b} <= 2'b11; + end + + initial begin + #30; + `checks(a_log, "1@5 0@7 1@15 ") + `checks(b_log, "1@5 0@7 1@15 ") + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_clocking_drive_same.py b/test_regress/t/t_clocking_drive_same.py new file mode 100755 index 000000000..620c6ea6b --- /dev/null +++ b/test_regress/t/t_clocking_drive_same.py @@ -0,0 +1,18 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Synchronous drives of the value last driven by the clocking block +# +# 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"]) + +test.execute() + +test.passes() diff --git a/test_regress/t/t_clocking_drive_same.v b/test_regress/t/t_clocking_drive_same.v new file mode 100644 index 000000000..1cd887360 --- /dev/null +++ b/test_regress/t/t_clocking_drive_same.v @@ -0,0 +1,169 @@ +// DESCRIPTION: Verilator: Synchronous drives of the value last driven by the clocking block +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// verilog_format: off +`define stop $stop +`define checks(gotv,expv) do if ((gotv) != (expv)) begin $write("%%Error: %s:%0d: got=\"%s\" exp=\"%s\"\n", `__FILE__,`__LINE__, (gotv), (expv)); `stop; end while(0); +// verilog_format: on + +interface bus_if ( + input bit clk +); + bit w; + clocking cb @(posedge clk); + output w; + endclocking + modport tb(clocking cb); +endinterface + +class Driver; + virtual bus_if vif; + virtual bus_if.tb mvif; + virtual bus_if vifs[2]; + int idx; + task run(); + repeat (2) begin + @(vif.cb); + vif.cb.w <= 1; + mvif.cb.w <= 1; + vifs[idx].cb.w <= 1; + end + endtask +endclass + +module sub ( + input bit clk +); + bit s; + clocking scb @(posedge clk); + output s; + endclocking +endmodule + +module t; + bit clk; + bit en = 1; + bit k; + bit kd; // Driven with a cycle delay + bit ki; // Driven conditionally + bit ks; // Driven with an output skew + string k_log; + string kd_log; + string ki_log; + string ks_log; + string s_log; + string s0_log; + string s1_log; + string w_log; + string wm_log; + string w0_log; + string w1_log; + Driver drv = new; + + always #5 clk = ~clk; + + default clocking pe @(posedge clk); + output k, kd, ki; + output #1 ks; + endclocking + + bus_if bus (.clk); + bus_if mbus (.clk); + bus_if buses[2] (.clk); + sub sub (.clk); + sub subs[2] (.clk); + + function automatic void log(inout string s, input bit v); + if ($time != 0) s = {s, $sformatf("%0d@%0d ", v, $time)}; + endfunction + + always @(k) log(k_log, k); + always @(kd) log(kd_log, kd); + always @(ki) log(ki_log, ki); + always @(ks) log(ks_log, ks); + always @(sub.s) log(s_log, sub.s); + always @(subs[0].s) log(s0_log, subs[0].s); + always @(subs[1].s) log(s1_log, subs[1].s); + always @(bus.w) log(w_log, bus.w); + always @(mbus.w) log(wm_log, mbus.w); + always @(buses[0].w) log(w0_log, buses[0].w); + always @(buses[1].w) log(w1_log, buses[1].w); + + // A drive of the value last driven by the clocking block also assigns the signal, after it + // was changed by a procedural assignment (IEEE 1800-2023 14.16.2) + initial begin + @(pe); + pe.k <= 1; + pe.ks <= 1; + sub.scb.s <= 1; + subs[1].scb.s <= 1; + buses[0].cb.w <= 1; + #2; + k = 0; + ks = 0; + sub.s = 0; + subs[1].s = 0; + bus.w = 0; + mbus.w = 0; + buses[0].w = 0; + buses[1].w = 0; + @(pe); + pe.k <= 1; + pe.ks <= 1; + sub.scb.s <= 1; + subs[1].scb.s <= 1; + buses[0].cb.w <= 1; + end + + // A drive with a cycle delay assigns the signal only when it matures + initial begin + @(pe); + pe.kd <= ##1 1; + @(pe); + #2 kd = 0; + @(pe); + pe.kd <= ##1 1; + end + + // A conditional drive assigns the signal only when executed + initial begin + @(pe); + if (en) pe.ki <= 1; + #2 ki = 0; + en = 0; + @(pe); + if (en) pe.ki <= 1; + en = 1; + @(pe); + if (en) pe.ki <= 1; + end + + initial begin + drv.vif = bus; + drv.mvif = mbus; + drv.vifs[0] = buses[0]; + drv.vifs[1] = buses[1]; + drv.idx = 1; + drv.run(); + end + + initial begin + #50; + `checks(k_log, "1@5 0@7 1@15 ") + `checks(kd_log, "1@15 0@17 1@35 ") + `checks(ki_log, "1@5 0@7 1@25 ") + `checks(ks_log, "1@6 0@7 1@16 ") + `checks(s_log, "1@5 0@7 1@15 ") + `checks(s0_log, "") + `checks(s1_log, "1@5 0@7 1@15 ") + `checks(w_log, "1@5 0@7 1@15 ") + `checks(wm_log, "1@5 0@7 1@15 ") + `checks(w0_log, "1@5 0@7 1@15 ") + `checks(w1_log, "1@5 0@7 1@15 ") + $write("*-* All Finished *-*\n"); + $finish; + end +endmodule diff --git a/test_regress/t/t_lint_sideeffect_bad.out b/test_regress/t/t_lint_sideeffect_bad.out index a5ab82daf..45bd562da 100644 --- a/test_regress/t/t_lint_sideeffect_bad.out +++ b/test_regress/t/t_lint_sideeffect_bad.out @@ -16,4 +16,14 @@ : ... Suggest use a temporary variable in place of this expression 17 | arr[postincrement_i()][postincrement_i()]++; | ^~~~~~~~~~~~~~~ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:29:9: Expression side effect may be mishandled + : ... note: In instance 't' + : ... Suggest use a temporary variable in place of this expression + 29 | vifs[postincrement_i()].cb.w <= 1; + | ^ +%Warning-SIDEEFFECT: t/t_lint_sideeffect_bad.v:29:10: Expression side effect may be mishandled + : ... note: In instance 't' + : ... Suggest use a temporary variable in place of this expression + 29 | vifs[postincrement_i()].cb.w <= 1; + | ^~~~~~~~~~~~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_lint_sideeffect_bad.v b/test_regress/t/t_lint_sideeffect_bad.v index 08ed250f5..1394e7a5e 100644 --- a/test_regress/t/t_lint_sideeffect_bad.v +++ b/test_regress/t/t_lint_sideeffect_bad.v @@ -17,4 +17,24 @@ module t; arr[postincrement_i()][postincrement_i()]++; $display("Value: %d", i); end + + bit clk; + bus_if buses[2] (.clk); + virtual bus_if vifs[2]; + + initial begin + vifs[0] = buses[0]; + vifs[1] = buses[1]; + // The interface is evaluated again to note the synchronous drive + vifs[postincrement_i()].cb.w <= 1; + end endmodule + +interface bus_if ( + input bit clk +); + bit w; + clocking cb @(posedge clk); + output w; + endclocking +endinterface