diff --git a/docs/guide/warnings.rst b/docs/guide/warnings.rst index 0ed315330..923ed4d0e 100644 --- a/docs/guide/warnings.rst +++ b/docs/guide/warnings.rst @@ -96,21 +96,21 @@ List Of Warnings .. option:: ALWCOMBORDER - .. TODO better example - - Warns that an ``always_comb`` block has a variable that is set after it - is used. This may cause simulation-synthesis mismatches, as not all - simulators allow this ordering. + Warns that an ``always_comb`` block reads a variable before assigning it + later in the same block. Because statements in a procedural block execute + in source order, the read observes the variable's previous value for that + activation. This can imply latch/state-like behavior and is not purely + combinational. .. code-block:: sv always_comb begin - a = b; - b = 1; + y = a + tmp; // Reads the previous value of tmp + tmp = b; end - Ignoring this warning will only suppress the lint check; it will - simulate correctly. + If the new value of ``tmp`` is intended, assign ``tmp`` before reading it. + Suppressing this warning may make results vary between simulators. .. option:: ALWNEVER diff --git a/src/V3Undriven.cpp b/src/V3Undriven.cpp index 37c514a21..5d76029a2 100644 --- a/src/V3Undriven.cpp +++ b/src/V3Undriven.cpp @@ -42,6 +42,7 @@ class UndrivenVarEntry final { AstVar* const m_varp; // Variable this tracks std::vector m_wholeFlags; // Used/Driven on whole vector std::vector m_bitFlags; // Used/Driven on each subbit + const AstNode* m_usedNotDrivenp = nullptr; // First read before any write const AstAlways* m_alwCombp = nullptr; // always_comb of var if driven within always_comb, else nullptr const FileLine* m_alwCombFileLinep = nullptr; // File line of always_comb of var if driven @@ -82,6 +83,10 @@ private: bool drivenFlag(int bit) const { return m_wholeFlags[FLAG_DRIVEN] || m_bitFlags[bit * FLAGS_PER_BIT + FLAG_DRIVEN]; } + int bitCount() const { return m_bitFlags.size() / FLAGS_PER_BIT; } + void recordUsedNotDriven(const AstNode* nodep) { + if (!m_usedNotDrivenp) m_usedNotDrivenp = nodep; + } enum BitNamesWhich : uint8_t { BN_UNUSED, BN_UNDRIVEN, BN_BOTH }; string bitNames(BitNamesWhich which) { string bits; @@ -121,6 +126,14 @@ private: public: void usedWhole(const AstNode* nodep) { UINFO(9, "set u[*] " << m_varp->name() << " " << nodep); + if (!m_wholeFlags[FLAG_DRIVEN]) { + for (int bit = 0; bit < bitCount(); ++bit) { + if (!drivenFlag(bit)) { + recordUsedNotDriven(nodep); + break; + } + } + } m_wholeFlags[FLAG_USED] = true; } void drivenWhole(const AstNode* nodep) { @@ -156,10 +169,13 @@ public: const FileLine* getNodeFileLinep() const { return m_nodeFileLinep; } const AstAlways* getAlwCombp() const { return m_alwCombp; } const FileLine* getAlwCombFileLinep() const { return m_alwCombFileLinep; } - void usedBit(int bit, int width) { + void usedBit(int bit, int width, const AstNode* nodep) { UINFO(9, "set u[" << (bit + width - 1) << ":" << bit << "] " << m_varp->name()); for (int i = 0; i < width; i++) { - if (bitNumOk(bit + i)) m_bitFlags[(bit + i) * FLAGS_PER_BIT + FLAG_USED] = true; + if (bitNumOk(bit + i)) { + if (!drivenFlag(bit + i)) recordUsedNotDriven(nodep); + m_bitFlags[(bit + i) * FLAGS_PER_BIT + FLAG_USED] = true; + } } } void drivenBit(int bit, int width) { @@ -181,6 +197,7 @@ public: bool isUsedNotDrivenAny() const { return isUsedNotDrivenBit(0, m_bitFlags.size() / FLAGS_PER_BIT); } + const AstNode* firstUsedNotDrivenp() const { return m_usedNotDrivenp; } static bool unusedMatch(AstVar* nodep) { const string regexp = v3Global.opt.unusedRegexp(); if (regexp == "") return false; @@ -374,7 +391,7 @@ class UndrivenVisitor final : public VNVisitorConst { } } - void warnAlwCombOrder(AstNodeVarRef* nodep) { + void warnAlwCombOrder(AstNodeVarRef* nodep, const AstNode* readp) { AstVar* const varp = nodep->varp(); if (!varp->isParam() && !varp->isGenVar() && !varp->isUsedLoopIdx() && !varp->ignoreSchedWrite() @@ -382,8 +399,16 @@ class UndrivenVisitor final : public VNVisitorConst { && !VN_IS(nodep, VarXRef) // Xrefs might point at two different instances && !varp->fileline()->warnIsOff( V3ErrorCode::ALWCOMBORDER)) { // Warn only once per variable - nodep->v3warn(ALWCOMBORDER, - "Always_comb variable driven after use: " << nodep->prettyNameQ()); + nodep->v3warn( + ALWCOMBORDER, + "always_comb reads " << nodep->prettyNameQ() + << " before assigning it later in the same block; behavior " + "may imply latch/state-like behavior and is not purely " + "combinational" + << (readp ? "\n" + readp->warnOther() + + "... Location of earlier read\n" + + readp->warnContextSecondary() + : "")); varp->fileline()->modifyWarnOff(V3ErrorCode::ALWCOMBORDER, true); // Complain just once for any usage } @@ -433,12 +458,12 @@ class UndrivenVisitor final : public VNVisitorConst { if (usr == 2 && m_alwaysCombp && entryp->isUsedNotDrivenBit(lsb, nodep->width())) { UINFO(9, " Select. Entryp=" << cvtToHex(entryp)); - warnAlwCombOrder(varrefp); + warnAlwCombOrder(varrefp, entryp->firstUsedNotDrivenp()); } entryp->drivenBit(lsb, nodep->width()); } if (m_inBBox || !varrefp->access().isWriteOrRW()) - entryp->usedBit(lsb, nodep->width()); + entryp->usedBit(lsb, nodep->width(), varrefp); } } else { // else other varrefs handled as unknown mess in AstVarRef @@ -487,7 +512,7 @@ class UndrivenVisitor final : public VNVisitorConst { if (m_inBBox || nodep->access().isWriteOrRW()) { if (usr == 2 && m_alwaysCombp && entryp->isUsedNotDrivenAny()) { UINFO(9, " Full bus. Entryp=" << cvtToHex(entryp)); - warnAlwCombOrder(nodep); + warnAlwCombOrder(nodep, entryp->firstUsedNotDrivenp()); } if (entryp->isDrivenWhole() && !m_inBBox && !VN_IS(nodep, VarXRef) && !VN_IS(nodep->dtypep()->skipRefp(), UnpackArrayDType) diff --git a/test_regress/t/t_lint_always_comb_bad.out b/test_regress/t/t_lint_always_comb_bad.out index 9aefafaf9..2330a07c3 100644 --- a/test_regress/t/t_lint_always_comb_bad.out +++ b/test_regress/t/t_lint_always_comb_bad.out @@ -7,8 +7,11 @@ : ... note: In instance 't' 31 | temp1 = (temp1_d1r - 'h1); | ^~~~~ -%Warning-ALWCOMBORDER: t/t_lint_always_comb_bad.v:32:5: Always_comb variable driven after use: 'mid' +%Warning-ALWCOMBORDER: t/t_lint_always_comb_bad.v:32:5: always_comb reads 'mid' before assigning it later in the same block; behavior may imply latch/state-like behavior and is not purely combinational : ... note: In instance 't' + t/t_lint_always_comb_bad.v:28:9: ... Location of earlier read + 28 | if (mid) + | ^~~ 32 | mid = (temp1_d1r == 'h0); | ^~~ ... For warning description see https://verilator.org/warn/ALWCOMBORDER?v=latest diff --git a/test_regress/t/t_lint_always_comb_order_bad.out b/test_regress/t/t_lint_always_comb_order_bad.out new file mode 100644 index 000000000..a3a9a4b17 --- /dev/null +++ b/test_regress/t/t_lint_always_comb_order_bad.out @@ -0,0 +1,17 @@ +%Warning-ALWCOMBORDER: t/t_lint_always_comb_order_bad.v:24:5: always_comb reads 'accum1' before assigning it later in the same block; behavior may imply latch/state-like behavior and is not purely combinational + : ... note: In instance 't' + t/t_lint_always_comb_order_bad.v:23:19: ... Location of earlier read + 23 | result1 = a + accum1; + | ^~~~~~ + 24 | accum1 = b; + | ^~~~~~ + ... For warning description see https://verilator.org/warn/ALWCOMBORDER?v=latest + ... Use "/* verilator lint_off ALWCOMBORDER */" and lint_on around source to disable this message. +%Warning-ALWCOMBORDER: t/t_lint_always_comb_order_bad.v:44:5: always_comb reads 'accum3' before assigning it later in the same block; behavior may imply latch/state-like behavior and is not purely combinational + : ... note: In instance 't' + t/t_lint_always_comb_order_bad.v:40:21: ... Location of earlier read + 40 | result3 = a + accum3; + | ^~~~~~ + 44 | accum3 = b; + | ^~~~~~ +%Error: Exiting due to diff --git a/test_regress/t/t_lint_always_comb_order_bad.py b/test_regress/t/t_lint_always_comb_order_bad.py new file mode 100644 index 000000000..a21d02ed5 --- /dev/null +++ b/test_regress/t/t_lint_always_comb_order_bad.py @@ -0,0 +1,16 @@ +#!/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('vlt') + +test.compile(fails=True, expect_filename=test.golden_filename) + +test.passes() diff --git a/test_regress/t/t_lint_always_comb_order_bad.v b/test_regress/t/t_lint_always_comb_order_bad.v new file mode 100644 index 000000000..dfbb2cfe0 --- /dev/null +++ b/test_regress/t/t_lint_always_comb_order_bad.v @@ -0,0 +1,47 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Zhi QU +// SPDX-License-Identifier: CC0-1.0 + +module t ( + input logic [3:0] a, + input logic [3:0] b, + input logic [3:0] c, + input logic sel, + output logic [3:0] result1, + output logic [3:0] result2, + output logic [3:0] result3, + output logic [3:0] y +); + + logic [3:0] accum1; + logic [3:0] accum2; + logic [3:0] accum3; + + always_comb begin + result1 = a + accum1; + accum1 = b; + end + + always_comb begin + accum2 = b; + result2 = a + accum2; // write-before-read: do not warn + end + + always_comb begin + y = '0; + if (sel) y = a; + else y = b; + end + + always_comb begin + if (sel) begin + result3 = a + accum3; + end else begin + result3 = c; + end + accum3 = b; + end + +endmodule