luke/verilator
1
0
Fork 0
mirror of https://github.com/verilator/verilator.git synced 2026-10-06 01:54:01 +02:00
Code Issues Packages Projects Releases Wiki Activity

Fix synchronous drives of an unchanged value not assigning the signal (#8486)

Browse Source
This commit is contained in:
Marco Bartoli
2026-10-03 09:59:24 -04:00
committed by GitHub
parent 9f72509635
commit 256468e8ae
8 changed files with 371 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/V3AssertPre.cpp
+88 -22
View File
@@ -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<SynchDrive> m_drives; // Clockvars written by the synchronous drive
bool m_hasCycleDelay = false; // True if node has cycle delay beneath
std::vector<AstVarXRef*> m_xrefsp; // list of xrefs that need name fixup
std::vector<AstSequence*> 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 {
src/V3AstAttr.h
+1
View File
@@ -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; }
test_regress/t/t_clocking_drive_concat.py
Executable
+18
View File
@@ -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()
test_regress/t/t_clocking_drive_concat.v
+47
View File
@@ -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
test_regress/t/t_clocking_drive_same.py
Executable
+18
View File
@@ -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()
test_regress/t/t_clocking_drive_same.v
+169
View File
@@ -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
test_regress/t/t_lint_sideeffect_bad.out
+10
View File
@@ -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
test_regress/t/t_lint_sideeffect_bad.v
+20
View File
@@ -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