Verilog globals appended to modules instead of prepended

jmi2k commented 4 weeks ago

Version

Yosys 0.46+11 (git sha1 0200a7680, clang++ 14.0.0-1ubuntu1.1 -fPIC -O3)

On which OS did this happen?

Linux

Reproduction Steps

Minimal reproducible example SoC.sv:

typedef struct packed {
    logic y;
    logic x;
} Vec_2_B;

module Gpu (output logic mixed_sync);
endmodule

(* top *)
module Top(output Vec_2_B vga_sync);

    Gpu gpu(vga_sync.x);

endmodule

Build command:

yosys -p 'synth_ecp5 -abc2' SoC.sv

Expected Behavior

The SV file is accepted without any error messages.

Actual Behavior

An assertion fails:

Generating RTLIL representation for module `\Top'.
ERROR: Assert `!source_offset && !id2ast->range_swapped' failed in frontends/ast/genrtlil.cc:1604.

jmi2k commented 4 weeks ago

Found an even simpler example, which indicates the issue manifests when using struct fields anywhere:

typedef struct packed {
    logic y;
    logic x;
} Vec_2_B;

(* top *)
module Top;

    Vec_2_B two_dee;
    wire foo = two_dee.x;

endmodule

jmi2k commented 4 weeks ago

Another hint: this other example seems to work fine.

(* top *)
module Top;

    typedef struct packed {
        logic y;
        logic x;
    } Vec_2_B;

    Vec_2_B two_dee;
    wire foo = two_dee.x;

endmodule

jmi2k commented 4 weeks ago

More hints: dumping the AST for both cases reveals only a single meaningful difference: the placement of the AST_TYPEDEF node inside the AST_MODULE. Note how, when the typedef is inside, it appears in the correct location (before everything else), but when it's outside it gets appended to the module contents.

typedef outside module:

Dumping AST before simplification:
    AST_MODULE <rtl/SoC.sv:7.1-12.10> [0x59dc00d2ad00] str='\Top'
      ATTR \top:
        AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59dc00d2ab90] bits='00000000000000000000000000000001'(32) range=[31:0] int=1 in_param
      AST_WIRE <rtl/SoC.sv:9.10-9.17> [0x59dc00d2b0c0] str='\two_dee' logic
        AST_WIRETYPE <rtl/SoC.sv:0.0-0.0> [0x59dc00d2b1f0] str='\Vec_2_B' in_param
      AST_WIRE <rtl/SoC.sv:10.7-10.10> [0x59dc00d2af90] str='\foo'
      AST_ASSIGN <rtl/SoC.sv:10.7-10.22> [0x59dc00d3b720]
        AST_IDENTIFIER <rtl/SoC.sv:0.0-0.0> [0x59dc00d3b5f0] str='\foo' in_lvalue
        AST_IDENTIFIER <rtl/SoC.sv:10.13-10.22> [0x59dc00d2b340] str='\two_dee.x'
      AST_TYPEDEF <rtl/SoC.sv:0.0-0.0> [0x59dc00d2b970] str='\Vec_2_B'
        AST_STRUCT <rtl/SoC.sv:0.0-0.0> [0x59dc00d2baa0]
          AST_STRUCT_ITEM <rtl/SoC.sv:2.8-2.9> [0x59dc00d2bbf0] str='y' logic
          AST_STRUCT_ITEM <rtl/SoC.sv:3.8-3.9> [0x59dc00d2bd20] str='x' logic
--- END OF AST DUMP ---

typedef inside module:

Dumping AST before simplification:
    AST_MODULE <rtl/SoC.sv:2.1-12.10> [0x61ba626cd9a0] str='\Top'
      ATTR \top:
        AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x61ba626cd830] bits='00000000000000000000000000000001'(32) range=[31:0] int=1 in_param
      AST_TYPEDEF <rtl/SoC.sv:0.0-0.0> [0x61ba626fab90] str='\Vec_2_B'
        AST_STRUCT <rtl/SoC.sv:0.0-0.0> [0x61ba626cdaf0]
          AST_STRUCT_ITEM <rtl/SoC.sv:5.9-5.10> [0x61ba626facf0] str='y' logic
          AST_STRUCT_ITEM <rtl/SoC.sv:6.9-6.10> [0x61ba626fae20] str='x' logic
      AST_WIRE <rtl/SoC.sv:9.10-9.17> [0x61ba626fb1f0] str='\two_dee' logic
        AST_WIRETYPE <rtl/SoC.sv:0.0-0.0> [0x61ba626fb340] str='\Vec_2_B' in_param
      AST_WIRE <rtl/SoC.sv:10.7-10.10> [0x61ba626fb0a0] str='\foo'
      AST_ASSIGN <rtl/SoC.sv:10.7-10.22> [0x61ba6270b860]
        AST_IDENTIFIER <rtl/SoC.sv:0.0-0.0> [0x61ba6270b730] str='\foo' in_lvalue
        AST_IDENTIFIER <rtl/SoC.sv:10.13-10.22> [0x61ba626fb4c0] str='\two_dee.x'
--- END OF AST DUMP ---

jmi2k commented 4 weeks ago

Tried the obvious thing after seeing both ASTs (getting the global Verilog definitions to be at the beginning of the module definition instead of the end) and it seems to solve not only this particular problem, but apparently any usage of them in general (even if I didn't trigger this particular issue, global typedef structs were unusable before).

So, in frontends/ast/ast.cc:1397, changing:

        // [...]
        if (child->type == AST_MODULE || child->type == AST_INTERFACE)
        {
            for (auto n : design->verilog_globals)
                child->children.push_back(n->clone());
        // [...]

to:

        // [...]
        if (child->type == AST_MODULE || child->type == AST_INTERFACE)
        {
            for (auto n : design->verilog_globals)
                child->children.insert(child->children.begin(), n->clone());
        // [...]

seems to work fine. I'll make a PR for it ASAP, although I don't know whether this fix is just hiding a deeper problem. In any case, that's not a decision for me to take I guess.

YosysHQ / yosys