Fix overzealous LATCH warning with procedure-local vars

[tsailer]

The LATCH warning seems rather overzealous in Verilator 4.108 2021-01-10 rev v4.106-158-g484b76e5b; for the code below, it complains about n, but n is assigned on all paths where it is actually defined.

t_latch.sv

[wsnyder]

Showing code here for easier viewing/discussion:

module test
  (input  logic       a,
   input  logic [7:0] b,
   output logic [7:0] c);

   always_comb begin : p
      c = b;
      if (a) begin : x
         logic [7:0] n;
         n = b;
         n += 8'h01;
         c = n;
      end : x
   end : p
endmodule

[wsnyder]

You are correct the current code improperly warns. Perhaps you'd be willing to look at the code and make a pull request to fix this? Otherwise perhaps @margej you might be interested?

[riteme]

Is this issue specific to procedure-local vars? I found two examples with no procedure-local vars, where Verilator seems to report improper LATCH warnings.

Latch1.sv:

module Latch1(
    input logic en,
    input logic a, b,
    output logic [1:0] c
);
    always_comb begin
        if (en) begin
            c = 2'b0;
        end else begin
            c[1] = a;
            c[0] = b;
        end
    end
endmodule

Latch2.sv (similar to Latch1.sv, but logic [1:0] is replaced with a packed struct):

typedef struct packed {
    logic a;
    logic b;
} twin_t;

module Latch2(
    input logic en,
    input logic a, b,
    output twin_t c
);
    always_comb begin
        if (en) begin
            {c.a, c.b} = 2'b0;
        end else begin
            c.a = a;
            c.b = b;
        end
    end
endmodule

Both can be temporarily fixed by making a local copy of c and putting /* verilator split_var */ on it.

Edit: I'm using Verilator 4.200 2021-03-12 rev v4.200.

[deo248]

I'm also seeing that this seems to not be specific to procedure-local variables as I'm getting what I believe to be the same spurious warning as above where the nets in question are not procedure-local (a mix of an interface port and some module-scope variables). Let me know if an additional test case would help, though it seems you have a few already here.

[margej]

Latch1.sv and Latch2.sv above from @riteme are more instances of #2938
The original issue from @tsailer is a different issue which is not resolved by the #2938 fix

[margej]

The following even simpler (albeit nonsensical) test case also emits a latch warning:

module test
  (input  logic       a,
   input  logic [7:0] b);

   always_comb begin : p
      if (a) begin : x
         logic [7:0] n;
         n = b;
      end : x
   end : p
endmodule

The reason being the rule set for latch detection is that all branches of a combinational always must contain at least one assignment to every output driven therein. The design does not consider the possibility that the scope of an 'output' can be limited to a particular branch of the block, therefore it expects to find an assignment to 'n' in the else clause of this if, even though that would be impossible. I'll have a think about how to fix ...

[margej]

I have a fix which excludes from latch detection any variables which are declared after (higher firstLineno()) the always being analysed. Would that be an acceptable fix?

[margej]

--- a/src/V3Active.cpp
+++ b/src/V3Active.cpp
@@ -317,11 +317,12 @@ private:
     const VNUser1InUse m_inuser1;
     // STATE
     LatchDetectGraph m_graph;  // Graph used to detect latches in combo always
+    int              m_lineno; // firstLineno() for the always being analysed
     // VISITORS
     void visit(AstVarRef* nodep) override {
         const AstVar* const varp = nodep->varp();
         if (nodep->access().isWriteOrRW() && varp->isSignal() && !varp->isUsedLoopIdx()
-            && !varp->isFuncLocalSticky()) {
+            && !varp->isFuncLocalSticky() && (varp->fileline()->firstLineno() <= m_lineno)) {
             m_graph.addAssignment(nodep);
         }
     }
@@ -345,6 +346,7 @@ private:
 public:
     // CONSTRUCTORS
     ActiveLatchCheckVisitor(AstNode* nodep, bool expectLatch) {
+        m_lineno = nodep->fileline()->firstLineno();
         m_graph.begin();
         iterateConst(nodep);
         m_graph.latchCheck(nodep, expectLatch);

[wsnyder]

We can't compare by line number, that will break in many ways, like with includes or any time earlier steps move things. It needs to track by position in tree by how the iteration is done.

[margej]

I'm glad you said that really because it felt like a hack! I did start out looking at tree position, but by Active all the VARs, including this local static, are attached directly to the top - Vt_lint_latch_8_049_active.tree:

MODULE so the original declaration position is not obvious.
     NETLIST 0x1569040 {a0aa}  $root [1ps/1ps]
    1: MODULE 0x15929b0 {d1ai}  __024root  L1 [P] [1ps]
    1:2: VAR 0x1593350 {d2ax} @dt=0x159e770@(G/w1)  a [PI] INPUT [P] PORT
    1:2: VAR 0x1591fd0 {d3ax} @dt=0x159fca0@(G/w8)  b [PI] INPUT [P] PORT
    1:2: VAR 0x159f6e0 {d2ax} @dt=0x159e770@(G/w1)  test__DOT__a PORT
    1:2: VAR 0x159ffe0 {d3ax} @dt=0x159fca0@(G/w8)  test__DOT__b PORT
    1:2: VAR 0x15a0c90 {d7aw} @dt=0x159fca0@(G/w8)  test__DOT__p__DOT__x__DOT__n [VSTATIC]  VAR
    1:2: TOPSCOPE 0x15954f0 {d1ai}
    1:2:2: SCOPE 0x1590150 {d1ai}  TOP [abovep=0] [cellp=0] [modp=0x15929b0]
    1:2:2:1: VARSCOPE 0x15955a0 {d2ax} @dt=0x159e770@(G/w1)  TOP->a [T] [scopep=0x1590150] -> VAR 0x1593350 {d2ax} @dt=0x159e770@(G/w1)  a [PI] INPUT [P] PORT
    1:2:2:1: VARSCOPE 0x159d6f0 {d3ax} @dt=0x159fca0@(G/w8)  TOP->b [T] [scopep=0x1590150] -> VAR 0x1591fd0 {d3ax} @dt=0x159fca0@(G/w8)  b [PI] INPUT [P] PORT
    1:2:2:1: VARSCOPE 0x1598140 {d2ax} @dt=0x159e770@(G/w1)  TOP->test__DOT__a [T] [scopep=0x1590150] -> VAR 0x159f6e0 {d2ax} @dt=0x159e770@(G/w1)  test__DOT__a PORT
    1:2:2:1: VARSCOPE 0x15907a0 {d3ax} @dt=0x159fca0@(G/w8)  TOP->test__DOT__b [T] [scopep=0x1590150] -> VAR 0x159ffe0 {d3ax} @dt=0x159fca0@(G/w8)  test__DOT__b PORT
    1:2:2:1: VARSCOPE 0x1597ae0 {d7aw} @dt=0x159fca0@(G/w8)  TOP->test__DOT__p__DOT__x__DOT__n [T] [scopep=0x1590150] -> VAR 0x15a0c90 {d7aw} @dt=0x159fca0@(G/w8)  test__DOT__p__DOT__x__DOT__n [VSTATIC]  VAR
    1:2:2:2: ACTIVE 0x1599aa0 {d2ax} => SENTREE 0x15999f0 {d2ax}
    1:2:2:2:1: SENTREE 0x15999f0 {d2ax}
    1:2:2:2:1:1: SENITEM 0x159b610 {d2ax} [COMBO]
    1:2:2:2:2: ASSIGNALIAS 0x159d7c0 {d2ax} @dt=0x159e770@(G/w1)
    1:2:2:2:2:1: VARREF 0x1583400 {d2ax} @dt=0x159e770@(G/w1)  a [RV] <- VARSCOPE 0x15955a0 {d2ax} @dt=0x159e770@(G/w1)  TOP->a [T] [scopep=0x1590150] -> VAR 0x1593350 {d2ax} @dt=0x159e770@(G/w1)  a [PI] INPUT [P] PORT
    1:2:2:2:2:2: VARREF 0x15834e0 {d2ax} @dt=0x159e770@(G/w1)  test__DOT__a [LV] => VARSCOPE 0x1598140 {d2ax} @dt=0x159e770@(G/w1)  TOP->test__DOT__a [T] [scopep=0x1590150] -> VAR 0x159f6e0 {d2ax} @dt=0x159e770@(G/w1)  test__DOT__a PORT
    1:2:2:2:2: ASSIGNALIAS 0x159d870 {d3ax} @dt=0x159fca0@(G/w8)
    1:2:2:2:2:1: VARREF 0x15835c0 {d3ax} @dt=0x159fca0@(G/w8)  b [RV] <- VARSCOPE 0x159d6f0 {d3ax} @dt=0x159fca0@(G/w8)  TOP->b [T] [scopep=0x1590150] -> VAR 0x1591fd0 {d3ax} @dt=0x159fca0@(G/w8)  b [PI] INPUT [P] PORT
    1:2:2:2:2:2: VARREF 0x1597fe0 {d3ax} @dt=0x159fca0@(G/w8)  test__DOT__b [LV] => VARSCOPE 0x15907a0 {d3ax} @dt=0x159fca0@(G/w8)  TOP->test__DOT__b [T] [scopep=0x1590150] -> VAR 0x159ffe0 {d3ax} @dt=0x159fca0@(G/w8)  test__DOT__b PORT
    1:2:2:2:2: ALWAYS 0x15908f0 {d5ae} [always_comb]
    1:2:2:2:2:2: IF 0x15909a0 {d6ah}
    1:2:2:2:2:2:1: VARREF 0x15956f0 {d6al} @dt=0x159e770@(G/w1)  a [RV] <- VARSCOPE 0x15955a0 {d2ax} @dt=0x159e770@(G/w1)  TOP->a [T] [scopep=0x1590150] -> VAR 0x1593350 {d2ax} @dt=0x159e770@(G/w1)  a [PI] INPUT [P] PORT
    1:2:2:2:2:2:2: ASSIGN 0x15957d0 {d8am} @dt=0x159fca0@(G/w8)
    1:2:2:2:2:2:2:1: VARREF 0x1595880 {d8ao} @dt=0x159fca0@(G/w8)  b [RV] <- VARSCOPE 0x159d6f0 {d3ax} @dt=0x159fca0@(G/w8)  TOP->b [T] [scopep=0x1590150] -> VAR 0x1591fd0 {d3ax} @dt=0x159fca0@(G/w8)  b [PI] INPUT [P] PORT
    1:2:2:2:2:2:2:2: VARREF 0x1597980 {d8ak} @dt=0x159fca0@(G/w8)  test__DOT__p__DOT__x__DOT__n [LV] => VARSCOPE 0x1597ae0 {d7aw} @dt=0x159fca0@(G/w8)  TOP->test__DOT__p__DOT__x__DOT__n [T] [scopep=0x1590150] -> VAR 0x15a0c90 {d7aw} @dt=0x159fca0@(G/w8)  test__DOT__p__DOT__x__DOT__n [VSTATIC]  VAR

It seems I'll need to go back to an earlier pass to capture the relative tree position of the VAR and the ALWAYS. It looks like its the Begin pass which moves the VAR outside the ALWAYS, which makes sense, I'll take a look in there ...

[margej]

I did find a "better" solution, but in hindsight, simply disabling detection for local variables is a cop-out, and could hide real latches. The real issue here is that the scope of the detection needs to match the scope of the original VAR declaration, whereas currently the scope of detection is the root of the ALWAYS containing the first assignment, so we get false +ves from branches of the ALWAYS that cannot possibly assign the VAR because its not in scope.

The proper fix would be to make the root of the latch detection graph equal to the node under which the VAR was originally declared. This will entail recording this postion in the VAR at the point it is 'lifted' in V3Begin.cpp, as well as a modification to the latch graph generation in V3Active.cpp