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

Deviation Between Yosys RTLIL EBNF Docs and RTLIL Lex/Yacc Frontend #4811

Open
ThePerfectComputer opened this issue Dec 10, 2024 · 5 comments
Labels
pending-verification This issue is pending verification and/or reproduction

Comments

@ThePerfectComputer
Copy link

Link to page

https://yosyshq.readthedocs.io/projects/yosys/en/latest/appendix/rtlil_text.html

Build number

Build #26545028

Issue

I'm currently working on a tool that ingests RTLIL, to this end
I've been referencing the RTLIL EBNF grammar found in the Yosys
docs as well as the RTLIL Lex/Yacc Yosys frontend code.

I can understand if RTLIL was not meant to be a source or target
for other tools, however, I think RTLIL may find new uses outside
of its original Yosys ecosystem. In light of this, I'd like to
draw attention to the following notable deviations I've
encountered that could have meaninful implications on how RTLIL
is interpreted for the tool I'm building.

Switch Statements in Process Body

As of Yosys commit 8148ebd, the Lex/Yacc RTLIL frontend allows
attribute statements, switch statements, and assignment statements
to appear in any order at the root level of a process body.
The relevant snippet of Yacc code can be found
here.

By contrast, the EBNF grammar doc as of commit 8148ebd allows
multiple switch statements at the root level of a process body,
but requires that all assignment statements occur before the
first switch statement. The EBNF grammar also effectively
requires that attribute statements be placed above their respectve
switch statement. In practice, this second deviation is not an
issue as I've never seen a tool that emits RTLIL that violates it.
The revelant snippet of the EBNF grammar can be found
here.

Assign statements in a process body to me indicate some sort of
default value. What would it mean for an assign statement to occur
after a switch value?

Furthermore, I also understand that the Lex/Yacc frontend is built
to be permissive and that just being able to call read_rtlil
doesn't mean the RTLIL source is valid. However, I have been able to
call read_rtlil, proc,and finally write_verilog on a file with
an assign_stmt occuring after a switch in a process body.

EBNF <module-body> Definition Presumably Missing <conn-stmt>

The current EBNF definition for <module-body> found in the Yosys
docs is:

 <module-body>       ::= (<param-stmt> 
                      |   <wire> 
                      |   <memory> 
                      |   <cell> 
                      |   <process>)*

Compare to the following from rtlil_parser.y:

module_body:
	module_body module_stmt |
	/* empty */;

module_stmt:
	param_stmt | param_defval_stmt | attr_stmt | wire_stmt | memory_stmt | cell_stmt | proc_stmt | conn_stmt;

The above effectively translates to:

module_body ::= (module_stmt)* ;
module_stmt ::= param_stmt | param_defval_stmt | attr_stmt | wire_stmt | memory_stmt | cell_stmt | proc_stmt | conn_stmt ;

I think the correct fix for the Yosys docs might look like:

 <module-body>       ::= (<param-stmt> 
                      |   <wire> 
                      |   <memory> 
                      |   <cell> 
                      |   <process>
                      |   <conn-stmt>)*

As of right now, <conn-stmt> is defined in the Yosys EBNF doc, but is not employed anywhere.

Expected

I'm not sure if assign statements should be allowed to appear after switches. With regards to module body, I think the definition should be update to the following:

 <module-body>       ::= (<param-stmt> 
                      |   <wire> 
                      |   <memory> 
                      |   <cell> 
                      |   <process>
                      |   <conn-stmt>)*

@ThePerfectComputer ThePerfectComputer added the pending-verification This issue is pending verification and/or reproduction label Dec 10, 2024
@georgerennie
Copy link
Collaborator

As of Yosys commit 8148ebd, the Lex/Yacc RTLIL frontend allows
attribute statements, switch statements, and assignment statements
to appear in any order at the root level of a process body.

This was discussed in #4765 that introduces 8148ebd. Previously both the EBNF and rtlil frontend allowed arbitrary ordering between assigns and switches, but the yosys internal datastructures did not capture this and would only represent things as if all assigns had come before the switches. We chose to emit a warning instead of an error when tightening up the frontend in case there are some users relying on this behaviour currently.

The parser rules require arbitrary ordering because otherwise the grammar would not be parseable without lookahead - it is not possible to distinguish <attribute> <switch> and <attribute> <assign> when you have just seen the <attribute> token. Although the grammar allows arbitrary ordering, there are assertions that actually enforce (or at least warn on) violations of the ordering

if (attrbuf.size() != 0)
rtlil_frontend_yyerror("dangling attribute");
// See https://github.com/YosysHQ/yosys/pull/4765 for discussion on this
// warning
if (!switch_stack.back()->empty()) {
rtlil_frontend_yywarning(
"case rule assign statements after switch statements may cause unexpected behaviour. "
"The assign statement is reordered to come before all switch statements."
);
}

As to the other issues, I would be happy to see PRs tightening up the EBNF docs/parser - I am sure there are plenty of places where they differ. The source of truth for what is allowed should come from the kernel/rtlil.h datastructures as those define what is actually representable internally.

@georgerennie
Copy link
Collaborator

I should also say that while much of RTLIL is probably suitable as an IL for other tools, I would be wary about using processes if you can at all help it. There have been various discussions about getting rid of processes as they have confusing semantics and inference rules. read_verilog is about the only place they are still generated, and it looks like yosys-slang (which doesn't use RTLIL processes nor the textual format) is likely to replace many end user uses of read_verilog.

@ThePerfectComputer
Copy link
Author

I know it is possible to squeeze processes out using the Yosys proc pass. The trouble I've found with this is I usually end up with a large amount of muxes.

It would be nice to both be able to squeeze out processes whilst retaining the higher level semantics present in switch in process.

@georgerennie
Copy link
Collaborator

Whoops i completely missed a word there, i mean sync processes in particular, comb only processes are slightly less problematic.

@whitequark
Copy link
Member

whitequark commented Dec 12, 2024

comb only processes are slightly less problematic.

Comb only processes are fine, or should be made fine if they somehow aren't. The decision of which columns to pick when generating a muxtree should not be the responsibility of each frontend individually.

I have affirmed this in the past discussions that were mentioned; I'm reproducing the information here for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-verification This issue is pending verification and/or reproduction
Projects
None yet
Development

No branches or pull requests

3 participants