Fix ALWCOMBORDER on variable ordering (#7350) (#7608)

docs/guide/warnings.rst

@@ -96,21 +96,21 @@ List Of Warnings
 
 .. option:: ALWCOMBORDER
 
-   .. TODO better example
+   Warns that an ``always_comb`` block reads a variable before assigning it
-
+   later in the same block. Because statements in a procedural block execute
-   Warns that an ``always_comb`` block has a variable that is set after it
+   in source order, the read observes the variable's previous value for that
-   is used. This may cause simulation-synthesis mismatches, as not all
+   activation. This can imply latch/state-like behavior and is not purely
-   simulators allow this ordering.
+   combinational.
 
    .. code-block:: sv
 
       always_comb begin
-        a = b;
+        y = a + tmp;  // Reads the previous value of tmp
-        b = 1;
+        tmp = b;
       end
 
-   Ignoring this warning will only suppress the lint check; it will
+   If the new value of ``tmp`` is intended, assign ``tmp`` before reading it.
-   simulate correctly.
+   Suppressing this warning may make results vary between simulators.
 
 
 .. option:: ALWNEVER

src/V3Undriven.cpp

@@ -42,6 +42,7 @@ class UndrivenVarEntry final {
     AstVar* const m_varp;  // Variable this tracks
     std::vector<bool> m_wholeFlags;  // Used/Driven on whole vector
     std::vector<bool> 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,
+            nodep->v3warn(
-                          "Always_comb variable driven after use: " << nodep->prettyNameQ());
+                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)

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

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

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()

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