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.

Sign up for GitHub

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

Jump to bottom

Wrong error message mentioning defparam when overriding localparam #4927

Open
marzoul opened this issue Mar 3, 2025 · 4 comments · May be fixed by #4930
Open

Wrong error message mentioning defparam when overriding localparam #4927

marzoul opened this issue Mar 3, 2025 · 4 comments · May be fixed by #4930
Labels
bug

Comments

@marzoul
Copy link

marzoul commented Mar 3, 2025 •
edited by KrystalDelusion
Loading

Version

Yosys 0.50+56 (git sha1 9106d6b, g++ 14.2.1 -march=x86-64 -mtune=generic -O2 -fno-plt -fexceptions -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffile-prefix-map=/home/prostboa/Dev/AUR/yosys-git/src=/usr/src/debug/yosys-git -flto=auto -fPIC -O3)

On which OS did this happen?

Linux

Reproduction Steps

Try to synthesize this (erroneous) design:

module comp #(
	localparam LP = 32
) (
	output wire [LP-1:0] odata
);

	assign odata = 0;

endmodule

module top #(
	parameter LP = 32
) (
	output wire [LP-1:0] oport
);

	wire [LP-1:0] owire;

	comp #(
		.LP(LP)
	 )
	comp_inst (
		.odata(owire)
	);

	assign oport = owire;

endmodule

Synthesize with this command:

yosys -l yosys.log -p "synth_xilinx -arch xc7 -top top ; write_json top.json" top.v

Expected Behavior

The error message should indicate that overriding localparams is illegal, and indicate the name of the related parameter.
So the root cause of the error can be found even in a very large design.

Actual Behavior

The error message mentions defparams, which is very misleading because defparam is a verilog keyword that is not used in the design.
The error message is extremely difficult to understand and does not indicate what parameter is related to the issue:

2.4.2. Executing AST frontend in derive mode using pre-parsed AST for module `\comp'.
Generating RTLIL representation for module `$paramod\comp'.
top.v:0: ERROR: Module name in defparam contains non-constant expressions!
@marzoul marzoul added the pending-verification This issue is pending verification and/or reproduction label Mar 3, 2025
@KrystalDelusion
Copy link
Member

KrystalDelusion commented Mar 6, 2025 •
edited
Loading

This appears to be specific to read_verilog -defer (which is what gets called when you pass in top.v on the command line like that). Actually I can also replicate it with read_verilog top.v; hierarchy -top top, but with calling hierarchy -check -auto-top (as it is in the synth scripts) correctly returns:

ERROR: Module `comp' referenced in module `top' in cell `comp_inst' does not have a parameter named 'LP'.

Or for extra fun, calling read_verilog -defer top.v; hierarchy -check also returns the expected error.

I suspect the pre-parsing to AST treats the parameter assignment as equivalent to a defparam, and doesn't distinguish between the two when it then tries to derive the design. hierarchy -check can (and should) pick up the error, but not if the module is $abstract and hierarchy is called with -top (or -auto-top).

@KrystalDelusion KrystalDelusion added bug and removed pending-verification This issue is pending verification and/or reproduction labels Mar 6, 2025
KrystalDelusion added a commit that referenced this issue Mar 6, 2025
@KrystalDelusion
tests: Add test for reassigning localparam
5748b37 
i.e. Wrong error message mentioning defparam when overriding localparam #4927
KrystalDelusion added a commit that referenced this issue Mar 6, 2025
@KrystalDelusion
hierarchy: Derive abstract cells in top mod
913ac04 
Previously abstract cells would only be derived if there was no top module declared, this means that `-check` would miss certain kinds of errors that it would normally detect.  This fixes #4927.
@KrystalDelusion KrystalDelusion linked a pull request Mar 6, 2025 that will close this issue
Draft
@KrystalDelusion KrystalDelusion linked a pull request Mar 6, 2025 that will close this issue
Derive abstract cells in top module during hierarchy #4930
Draft
@KrystalDelusion
Copy link
Member

KrystalDelusion commented Mar 7, 2025 •
edited
Loading

The work around here is to call (e.g.)

yosys -l yosys.log -p "read_verilog top.v; synth_xilinx -arch xc7 -top top; write_json top.json"

Turns out trying to get the missing parameter error from hierarchy -check while deriving the AST is much harder than I initially thought.

@marzoul
Copy link
Author

marzoul commented Mar 10, 2025

I realize there was actually another issue in the reported error message.
top.v:0: ERROR: Module name in defparam contains non-constant expressions!
In cases where this error is appropriate of course, it would be extremely welcome to display a meaningful line number to so that the source of the issue can be found. But I don't know how to create such a reproducer.

@KrystalDelusion
Copy link
Member

I think this might actually be better as an assertion error; I don't know how you would end up with a non-constant module name unless the AST was malformed so I don't think there is any case where this error is appropriate...

@KrystalDelusion KrystalDelusion marked this as a duplicate of #4955 Mar 21, 2025
@KrystalDelusion KrystalDelusion mentioned this issue Mar 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Derive abstract cells in top module during hierarchy YosysHQ/yosys
2 participants
@marzoul @KrystalDelusion