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

Generate region standard compliance fixes #239

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dwRchyngqxs
Copy link
Contributor

@dwRchyngqxs dwRchyngqxs commented May 23, 2023

Standard compliance fixes (Checked against Verilog 1995/2001/2005 and SystemVerilog 2017 A.4.2):

Do not accept generate regions inside other generate regions; this is now forbidden

module top();
  generate
    generate
    endgenerate
  endgenerate
endmodule

Do not print lone semicolon in generate region; this cannot be printed anymore

module top();
  generate
    ;
  endgenerate
endmodule

Print a block in for generate to be compatible with Verilog 2001

module top();
  for (...) begin
    ...
  end
endmodule

EDIT: Verilog standard are complicated because the syntax of a standard does not cover everything from the previous standard.
EDIT2: Added comments to explain why begin/end blocks are added in if and for.

@dwRchyngqxs dwRchyngqxs marked this pull request as draft May 23, 2023 20:50
@dwRchyngqxs
Copy link
Contributor Author

I accidentally prevented attributes on conditional and loop generate construct. I'm going to fixing that before going out of draft.

@dwRchyngqxs
Copy link
Contributor Author

Apparently attributes on generate regions are allowed in Verilog 2001, but Verilog 2005 is not a superset of Verilog 2001 so I didn't notice it before reading both standards... So in the end it's better to parse it but not print it.
There is still somethings to do in order to prevent an escalation from NonGenerateModuleItem to ModuleItem by putting an attribute. I guess I will convert this PR into fixing that bug and other non standard generate stuff printing.

@dwRchyngqxs dwRchyngqxs force-pushed the generate-no-attributes branch 2 times, most recently from 9f992cb to aba9ba7 Compare May 24, 2023 10:23
@dwRchyngqxs
Copy link
Contributor Author

dwRchyngqxs commented May 24, 2023

Ok, I think this is now correct. Only tests are left. EDIT: It is incorrect, the removed begin/end in if statements are actually useful for the dangling else problem.

@dwRchyngqxs dwRchyngqxs changed the title Forbid non-standard generate region Generate region standard compliance fixes May 24, 2023
@dwRchyngqxs dwRchyngqxs force-pushed the generate-no-attributes branch 3 times, most recently from 5fd2364 to 825b7c5 Compare May 31, 2023 19:20
qcorradi and others added 4 commits July 31, 2023 22:55
Standard compliance fixes (Checked against Verilog 1995/2001/2005 and SystemVerilog 2017 A.4.2):
Do not accept generate regions inside other generate regions
Accept attributes on all generate items independently from where they appear
Do not print lone semicolon in generate region
Print a block in for generate to be compatible with Verilog 2001
@zachjs
Copy link
Owner

zachjs commented Aug 1, 2023

I just pushed some revisions that should preserve most of the intended changes while fixing the issues raised in the test cases. What do you think?

@dwRchyngqxs
Copy link
Contributor Author

I've read your changes. They solve what I was trying to solve, but correctly.
Thank you for your help, I was unable to understand how sv2v handles generate regions and that was stalling the PR.

@zachjs zachjs marked this pull request as ready for review August 1, 2023 13:17
Copy link
Owner

@zachjs zachjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting these issues! This was a helpful PR.

@zachjs
Copy link
Owner

zachjs commented Aug 9, 2023

I realized that my changes actually break applying attributes to generate items, so I'll have to work on this further. I am considering adding a GIAttr akin to MIAttr, or potentially a larger refactor combining GenItem into ModuleItem.

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.

None yet

2 participants