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

Simulator compatibility #57

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

Conversation

heavySea
Copy link

@heavySea heavySea commented May 5, 2021

This pull requests contains fixes related to problems with simulation using Modelsim (and probably with other simulators too).

First commit:

Most design files of caracel use the Verilog-2001 port declaration style using implicit wire type declarations.
In cases where the port type must be changed, e.g. to define an output as reg, the type was declared inside the body of the module (à la Verilog-1995), instead of the header.
E.g. in user_id_programming.v:

module user_id_programming #(
    ..-
) (
    output [31:0] mask_rev
);

    wire [31:0] mask_rev;
    wire [31:0] user_proj_id_high;
    wire [31:0] user_proj_id_low;
    ....
endmodule

This seems to be a problem for some simulators, e.g. Modelsim, which think the wire [31:0] mask_rev; line declares a new wire. Since the name is already used in the scope of the module, this results in an error.
Thus, such cases where made to use the 2001-style port declaration only.

Second commit:

Solves #54

Third commit:

Suggested Fix for #55

Note: All requests do only change syntax / Simulation related parts of the design.

Most files use the Verilog-2001 port declaration style using implicit type declaration.
In cases where the port type must be changed, e.g. an output as reg, it was set in the body of the module (a là Verilog-1995), instead of the header after the direction specification.
E.g. in user_id_programming.v:
```
module user_id_programming #(
    ..-
) (
    output [31:0] mask_rev
);

    wire [31:0] mask_rev;
    wire [31:0] user_proj_id_high;
    wire [31:0] user_proj_id_low;
    ....
endmodule
```

This seems to be a problem for some simulators, e.g. Modelsim, which think the `wire [31:0] mask_rev;` line declares a new wire. because the name was already used in the scope this results in an error.
Thus such cases where made to use the 2001-style port declaration only.
Remove the compiler directives related to default port type
@olofk
Copy link

olofk commented May 5, 2021

I can confirm that this works fine with QuestaSim and Xilinx XSim after these changes.

If dropping default_nettype none is too controversial, I have already started the work to explicitly declare wires for all the verilog ports if that is preferred

@heavySea
Copy link
Author

heavySea commented May 6, 2021

I can confirm that this works fine with QuestaSim and Xilinx XSim after these changes.

If dropping default_nettype none is too controversial, I have already started the work to explicitly declare wires for all the verilog ports if that is preferred

Great! It is probably the safer solution.
Nevertheless, the directive must be removed in the testbenches, since the generated netlists do not include type declarations. (Example)

@olofk
Copy link

olofk commented May 6, 2021

Ouch, you're right. I didn't consider the generated netlists :/

I guess this should be reported to Yosys as well for the future, or maybe that is already done?

@mballance
Copy link

Ouch, you're right. I didn't consider the generated netlists :/

I guess this should be reported to Yosys as well for the future, or maybe that is already done?

I believe this is fine. The Yosys-generated netlists use the non-ANSI style for portlists -- untyped ports, with directions and types (eg wire) specified in the module body. The issue (IIRC) with some of the caravel IP was that it either mixed ANSI/non-ANSI ports within the same module, or specified both wire and reg for registered output ports.

@heavySea
Copy link
Author

heavySea commented May 6, 2021

The Yosys-generated netlists use the non-ANSI style for portlists -- untyped ports, with directions and types (eg wire) specified in the module body. <

Direction yes, but not the type. Thus ``default_nettype none` will generate errors.

The issue (IIRC) with some of the caravel IP was that it either mixed ANSI/non-ANSI ports within the same module, or specified both wire and reg for registered output ports.<

Yes, this is fixed with be2a8a6

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.

3 participants