Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepend Verilog globals to module AST #4656

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmi2k
Copy link

@jmi2k jmi2k commented Oct 11, 2024

What are the reasons/motivation for this change?

Fixes #4653. Further AST and RTLIL stages seem to be order-sensitive, and appending globals to the module children list did not work.

Explain how this is achieved.

Early during AST::process(), Verilog globals are prepended to the children list instead.

If applicable, please suggest to reviewers how they can test the change.

#4653 contains a test case that triggers an assertion, but works fine after applying this change.

@jmi2k jmi2k requested a review from zachjs as a code owner October 11, 2024 19:23
Fixes YosysHQ#4653. Further AST and RTLIL stages seem to be order-sensitive,
and appending globals to the module children list did not work.
@jmi2k
Copy link
Author

jmi2k commented Oct 11, 2024

This seems to have caused (or uncovered an issue with typedefd packed multidimensional arrays (in arrays03.sv). Commenting the affected cases unblocks the test suite up to this point.

[...]
Passed sign_array_query.ys
Passed size_cast.ys
Passed struct_access.ys
<<EOT:6: ERROR: syntax error, unexpected ';', expecting ATTR_BEGIN or TOK_INCREMENT or TOK_DECREMENT
Expected error pattern 'syntax error, unexpected ';', expecting ATTR_BEGIN or TOK_INCREMENT or TOK_DECREMENT' found !!!
Passed task_attr.ys
Passed typedef_across_files.ys
Passed typedef_const_shadow.ys
ERROR: Called with -verify and proof did fail!
make[1]: *** [run-test.mk:496: typedef_legacy_conflict.ys] Error 1
make[1]: Leaving directory '/home/jsanchez/Documents/Sources/yosys/tests/verilog'
make: *** [Makefile:898: test] Error 2

I will try to shed more light into this issue.

@jmi2k
Copy link
Author

jmi2k commented Oct 12, 2024

It seems like there's a mismatch in the resulting dimensions after simplification between declaring a 2D packed array of wires directly vs. using a typedef, where the former keeps the 2D dimensions but the latter collapses them into a single 1D range.

Example Verilog code used to test this behavior:

typedef logic [3:0][1:0] Matrix;

module Top;

`ifdef INLINE
	logic [3:0][1:0] matrix;
`else
	Matrix matrix;
`endif

	assign matrix = "a";

endmodule

Simplified AST when INLINE is defined:

Dumping AST after simplification:
    AST_MODULE <rtl/SoC.sv:3.1-13.10> [0x59d814aa2470] str='\Top' basic_prep
      AST_TYPEDEF <rtl/SoC.sv:0.0-0.0> [0x59d814a92ec0] str='\Matrix' basic_prep
        AST_WIRE <rtl/SoC.sv:0.0-0.0> [0x59d814a93010] logic basic_prep range=[7:0] dimensions=[3:0][1:0]
          AST_RANGE <rtl/SoC.sv:0.0-0.0> [0x59d814a93160] basic_prep range=[7:0] in_param
            AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59d814a93970] bits='00000000000000000000000000000111'(32) signed basic_prep range=[31:0] int=7 in_param
            AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59d814a936c0] bits='00000000000000000000000000000000'(32) signed basic_prep range=[31:0] in_param
      AST_WIRE <rtl/SoC.sv:8.9-8.15> [0x59d814aa2800] str='\matrix' logic basic_prep range=[7:0] dimensions=[7:0]
        ATTR \wiretype:
          AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59d814a95920] str='\Matrix' bits='01011100010011010110000101110100011100100110100101111000'(56) basic_prep range=[55:0] int=1953655160 in_param
        AST_RANGE <rtl/SoC.sv:0.0-0.0> [0x59d814a95570] basic_prep range=[7:0] in_param
          AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59d814a956c0] bits='00000000000000000000000000000111'(32) signed basic_prep range=[31:0] int=7 in_param
          AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x59d814a957f0] bits='00000000000000000000000000000000'(32) signed basic_prep range=[31:0] in_param
      AST_ASSIGN <rtl/SoC.sv:11.9-11.21> [0x59d814aa2a60] basic_prep
        AST_IDENTIFIER <rtl/SoC.sv:11.9-11.15> [0x59d814aa25a0 -> 0x59d814aa2800] str='\matrix' basic_prep in_lvalue
        AST_CONSTANT <rtl/SoC.sv:11.19-11.21> [0x59d814aa26d0] str='a' bits='01100001'(8) basic_prep range=[7:0] int=97
--- END OF AST DUMP ---

Simplified AST when INLINE is not defined:

Dumping AST after simplification:
    AST_MODULE <rtl/SoC.sv:3.1-13.10> [0x58a285b17360] str='\Top' basic_prep
      AST_TYPEDEF <rtl/SoC.sv:0.0-0.0> [0x58a285b17c20] str='\Matrix' basic_prep
        AST_WIRE <rtl/SoC.sv:0.0-0.0> [0x58a285b17d70] logic basic_prep range=[7:0] dimensions=[3:0][1:0]
          AST_RANGE <rtl/SoC.sv:0.0-0.0> [0x58a285b17ec0] basic_prep range=[7:0] in_param
            AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x58a285b18670] bits='00000000000000000000000000000111'(32) signed basic_prep range=[31:0] int=7 in_param
            AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x58a285b183c0] bits='00000000000000000000000000000000'(32) signed basic_prep range=[31:0] in_param
      AST_WIRE <rtl/SoC.sv:6.19-6.25> [0x58a285b27fa0] str='\matrix' logic basic_prep range=[7:0] dimensions=[3:0][1:0]
        AST_RANGE <rtl/SoC.sv:0.0-0.0> [0x58a285b28200] basic_prep range=[7:0] in_param
          AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x58a285b28350] bits='00000000000000000000000000000111'(32) signed basic_prep range=[31:0] int=7 in_param
          AST_CONSTANT <rtl/SoC.sv:0.0-0.0> [0x58a285b28480] bits='00000000000000000000000000000000'(32) signed basic_prep range=[31:0] in_param
      AST_ASSIGN <rtl/SoC.sv:11.9-11.21> [0x58a285b27ac0] basic_prep
        AST_IDENTIFIER <rtl/SoC.sv:11.9-11.15> [0x58a285b27d20 -> 0x58a285b27fa0] str='\matrix' basic_prep in_lvalue
        AST_CONSTANT <rtl/SoC.sv:11.19-11.21> [0x58a285b27bf0] str='a' bits='01100001'(8) basic_prep range=[7:0] int=97
--- END OF AST DUMP ---

Notice how the dimension property is [3:0][1:0] when declaring the type as a 2D packed array directly, but [7:0] when going through a typedef.

My guess is that the code below line 1944 of frontends/ast/simplify.cc is not right.

Honestly, I think I'm going to need some help here, so if somebody knows the internals better than me (so, knows them at all) I would appreciate some guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verilog globals appended to modules instead of prepended
1 participant