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

Add support for parsing the SystemVerilog 'bind' construct #2839

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

rswarbrick
Copy link
Contributor

This doesn't do anything useful yet: the patch just adds support for
the syntax to the lexer and parser and adds some tests to check the
syntax parses properly. This generates AST nodes, but doesn't yet
generate RTLIL.

Since our existing hierarchical_identifier parser doesn't allow bit
selects (so you can't do something like foo[1].bar[2].baz), I've also
not added support for a trailing bit select (the constant_bit_select
non-terminal in bind_target_instance in the spec). If we turn out to
need this in future, we'll want to augment hierarchical_identifier and
its other users too.

Note that you can't easily use the BNF from the spec:

bind_directive ::=
    "bind" bind_target_scope [ : bind_target_instance_list]
           bind_instantiation ;
  | "bind" bind_target_instance bind_instantiation ;

even if you fix the lookahead problem, because code like this matches
both branches in the BNF:

bind a b b_i (.*);

The problem is that 'a' could either be a module name or a degenerate
hierarchical reference. This seems to be a genuine syntactic
ambiguity, which the spec resolves (p739) by saying that we have to
wait until resolution time (the hierarchy pass) and take whatever is
defined, treating 'a' as an instance name if it names both an instance
and a module.

To keep the parser simple, it currently accepts this invalid syntax:

bind a.b : c d e (.*);

This is invalid because we're in the first branch of the BNF above, so
the a.b term should match bind_target_scope: a module or interface
identifier, not an arbitrary hierarchical identifier.

This will fail in the hierarchy pass (when it's implemented in a
future patch).

This is part of the commit queue in #2752, but has been split out to make reviewing easier.

@rswarbrick rswarbrick requested a review from zachjs as a code owner June 17, 2021 10:37
@rswarbrick
Copy link
Contributor Author

Hi all! It's been a month: is there anything I can do to help this get reviewed?

@zachjs
Copy link
Collaborator

zachjs commented Jul 15, 2021

Sorry for the delay! I'll review this today.

Copy link
Collaborator

@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.

This all seems very reasonable except for one minor comment. This doesn't introduce anything unprincipled and won't affect any existing designs.

}

cell_stmt:
just_cell_stmt |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change to cell_stmt necessary? The split out just_cell_stmt appears to only be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're completely right! I think this is the fallout from some code when I was first getting this working. Reverted.

This doesn't do anything useful yet: the patch just adds support for
the syntax to the lexer and parser and adds some tests to check the
syntax parses properly. This generates AST nodes, but doesn't yet
generate RTLIL.

Since our existing hierarchical_identifier parser doesn't allow bit
selects (so you can't do something like foo[1].bar[2].baz), I've also
not added support for a trailing bit select (the "constant_bit_select"
non-terminal in "bind_target_instance" in the spec). If we turn out to
need this in future, we'll want to augment hierarchical_identifier and
its other users too.

Note that you can't easily use the BNF from the spec:

    bind_directive ::=
        "bind" bind_target_scope [ : bind_target_instance_list]
               bind_instantiation ;
      | "bind" bind_target_instance bind_instantiation ;

even if you fix the lookahead problem, because code like this matches
both branches in the BNF:

    bind a b b_i (.*);

The problem is that 'a' could either be a module name or a degenerate
hierarchical reference. This seems to be a genuine syntactic
ambiguity, which the spec resolves (p739) by saying that we have to
wait until resolution time (the hierarchy pass) and take whatever is
defined, treating 'a' as an instance name if it names both an instance
and a module.

To keep the parser simple, it currently accepts this invalid syntax:

    bind a.b : c d e (.*);

This is invalid because we're in the first branch of the BNF above, so
the "a.b" term should match bind_target_scope: a module or interface
identifier, not an arbitrary hierarchical identifier.

This will fail in the hierarchy pass (when it's implemented in a
future patch).
@zachjs zachjs merged commit 414154d into YosysHQ:master Jul 16, 2021
@rswarbrick rswarbrick deleted the bind-parse branch July 16, 2021 13:50
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.

2 participants