assignment to member of array of struct not working in some constellations

[sumpster]

Some of my test suites started failing when upgrading from 5.020 to 5.030 and I tried to locate the breaking change. At first it looked like the culprit is this, because after this change my tests start to fail: (When I undo the change to V3Unknown.cpp the tests are green again.)

8c3cc3a#diff-bf2735b07bd0b382ff641879225a1d4957732b6fbfc5d4a91d77cf0668964f73R122

However, in the process of creating a small example of the problem for this bug report I ended up with the code below which is already not working for me in 5.020.

Problem: counter will not get incremented unless the line making the assignment to flag is commented out.

module demo_tb;
    typedef struct {
        logic [2:0] counter;
        logic flag;
    } SimpleStruct;

    SimpleStruct s [1];

    logic clock = 0;

    initial forever #5 clock = ~clock;

    always @(posedge clock) begin
        for (int i = 0; i < 1; i++) begin
            s[i].counter <= s[i].counter + 3'd1;
            s[i].flag <= 0; // <-- comment out this line and the test will pass
        end        
    end
    
    initial begin
        if (s[0].counter != 0) $stop;
        for (int i = 1; i < 8; i++)
            #10 if (s[0].counter != 3'(i)) $stop;

        $write("*-* All Finished *-*\n");
        $finish;
    end
endmodule

Reproduced with:

• Verilator 5.020
• Verilator 5.031 devel rev v5.030-78-g5470cf9fa

OS: Ubuntu 24.04.1 LTS
Run from cocotb 1.9 via python runner.

[wsnyder]

This example doesn't make sense to me, you test in some cases against index 4, it doesn't initialize the counter (do you want bit instead), and it's counting the number of clocks which is strange. Can you make an example that passes on other simulators please?

[wsnyder]

As to the V3Random, see #5605.

[sumpster]

• Its never using another index than 0. The first loop only does one iteration with i = 0; the second loop has index 0 hardcoded.
• The second loop is only used to test that the counter value increments each clock cycle - which is why its counting the number of clock cycles.
• Its an example reduced down to the trivial case of array size = 1 - which can happen when things are parameterized in the real code.

The always block is where the error happens, the rest is just to create a test which provides a clock and checks for the error.

The array is not initialized in the code, but according to the fst, counter defaults to 0. And the test for counter == 0 also passes in verilator. Its only when it checks for the expected increment that it fails.

The example also passes in xsim:

*-* All Finished *-*
$finish called at time : 70 ns : File "xyz/demo_tb.sv" Line 28
INFO: [USF-XSim-96] XSim completed. Design snapshot 'demo_tb_behav' loaded.

[sumpster]

Removed some unnecessary complexity from the example:

module demo_tb;
    typedef struct {
        logic a;
        logic b;
    } SimpleStruct;

    SimpleStruct s [1];

    logic clock;

    always @(posedge clock) begin
        for (int i = 0; i < 1; i++) begin
            s[i].a <= 1;
            s[i].b <= 0; // <-- comment out this line and the test will pass
        end        
    end
    
    initial begin
        clock = 0;
        s[0].a = 0;
        s[0].b = 0;

        #5 clock = 1;
        #1 if (s[0].a != 1) $stop;

        $write("*-* All Finished *-*\n");
        $finish;
    end
endmodule

Still passes in xsim. Fails in verilator unless the non-blocking assignment of b is commented out.

[sumpster]

As to the V3Random, see #5605.

Tried removing --trace-fst to see if it makes a difference as in the linked issue. (And in the process figured out how to run it independently of cocotb.) Here is the verilator command line I am using now: (With dummy impl for main.cpp)

verilator --timing --cc --exe --top-module demo_tb -o demo_tb --build demo_tb.sv main.cpp

It fails with the new example and passes when I comment out the non-blocking assignment to b.

[wsnyder]

Thanks, does seem identical issue to #5605, but will leave this open to test with that fix when it is ready, as failure is more obvious here.

[sumpster]

Took a bit of a look into verilators internal workings. Am I looking in the right place here?

VL_INLINE_OPT void Vdemo_tb___024root___nba_sequent__TOP__0(Vdemo_tb___024root* vlSelf) {
    (void)vlSelf;  // Prevent unused variable warning
    Vdemo_tb__Syms* const __restrict vlSymsp VL_ATTR_UNUSED = vlSelf->vlSymsp;
    VL_DEBUG_IF(VL_DBG_MSGF("+    Vdemo_tb___024root___nba_sequent__TOP__0\n"); );
    auto& vlSelfRef = std::ref(*vlSelf).get();
    // Init
    Vdemo_tb_demo_tb__DOT__SimpleStruct__struct__0 demo_tb__DOT____Vlvbound_h886d58da__0;
    demo_tb__DOT____Vlvbound_h886d58da__0.__PVT__a = 0;
    demo_tb__DOT____Vlvbound_h886d58da__0.__PVT__b = 0;
    Vdemo_tb_demo_tb__DOT__SimpleStruct__struct__0 demo_tb__DOT____Vlvbound_h886d58da__1;
    demo_tb__DOT____Vlvbound_h886d58da__1.__PVT__a = 0;
    demo_tb__DOT____Vlvbound_h886d58da__1.__PVT__b = 0;
    VlUnpacked<Vdemo_tb_demo_tb__DOT__SimpleStruct__struct__0, 1> __Vdly__demo_tb__DOT__s;
    for (int __Vi0 = 0; __Vi0 < 1; ++__Vi0) {
        __Vdly__demo_tb__DOT__s[__Vi0].__PVT__a = 0;
        __Vdly__demo_tb__DOT__s[__Vi0].__PVT__b = 0;
    }
    // Body
    __Vdly__demo_tb__DOT__s = vlSelfRef.demo_tb__DOT__s;
    demo_tb__DOT____Vlvbound_h886d58da__0 = vlSelfRef.demo_tb__DOT__s
        [0U];
    demo_tb__DOT____Vlvbound_h886d58da__0.__PVT__a = 1U;
    __Vdly__demo_tb__DOT__s[0U] = demo_tb__DOT____Vlvbound_h886d58da__0;
    demo_tb__DOT____Vlvbound_h886d58da__1 = vlSelfRef.demo_tb__DOT__s
        [0U];
    demo_tb__DOT____Vlvbound_h886d58da__1.__PVT__b = 0U;
    __Vdly__demo_tb__DOT__s[0U] = demo_tb__DOT____Vlvbound_h886d58da__1;
    vlSelfRef.demo_tb__DOT__s = __Vdly__demo_tb__DOT__s;
}

It looks like the change a = 1 is made in demo_tb__DOT____Vlvbound_h886d58da__0 which gets put in __Vdly__demo_tb__DOT__s[0U] but is overwritten by demo_tb__DOT____Vlvbound_h886d58da__1 which is not refreshed with the updated state, but an old one from vlSelfRef.demo_tb__DOT__s ultimately causing a = 1 to get lost.

What is the procedure supposed to look like? Is the problem that demo_tb__DOT____Vlvbound_h886d58da__1 is refreshed from the wrong source? Or should the changes (a = 1) be made directly in __Vdly__demo_tb__DOT__s?

I briefly looking into the AST tree. Is the problem caused by a wrong transformation in the tree? Where should I look when I am curious how to fix this?

[sumpster]

Oh, it looks like the transformation between a representation similar to my sv code and the convoluted (and incorrect) c++ code above happens in the AST tree from step 025_begin to 027_unknown.
With the change that triggered the errors in my tests being in V3Unknown.cpp, could the problematic transformation be happening there?

[sumpster]

Yes, when I uncomment the change from 8c3cc3a to V3Unknown.cpp, the generated code gets much simpler:

VL_INLINE_OPT void Vdemo_tb___024root___nba_sequent__TOP__0(Vdemo_tb___024root* vlSelf) {
    (void)vlSelf;  // Prevent unused variable warning
    Vdemo_tb__Syms* const __restrict vlSymsp VL_ATTR_UNUSED = vlSelf->vlSymsp;
    VL_DEBUG_IF(VL_DBG_MSGF("+    Vdemo_tb___024root___nba_sequent__TOP__0\n"); );
    auto& vlSelfRef = std::ref(*vlSelf).get();
    // Body
    vlSelfRef.demo_tb__DOT____Vlvbound_h886d58da__0.__PVT__a = 1U;
    vlSelfRef.demo_tb__DOT__s[0U] = vlSelfRef.demo_tb__DOT____Vlvbound_h886d58da__0;
    vlSelfRef.demo_tb__DOT____Vlvbound_h886d58da__1.__PVT__b = 0U;
    vlSelfRef.demo_tb__DOT__s[0U] = vlSelfRef.demo_tb__DOT____Vlvbound_h886d58da__1;
}

It still looks incorrect however, as a = 1 still gets overwritten in vlSelfRef.demo_tb__DOT__s[0U].
So perhaps the more complicated new version is more likely to trigger the problem, but it looks like the prior version is also broken for the sv test in this issue.