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

Spyglass issues with PeakRDL's CSR generated RTL #87

Closed
NajamKhalil opened this issue Mar 15, 2024 · 10 comments
Closed

Spyglass issues with PeakRDL's CSR generated RTL #87

NajamKhalil opened this issue Mar 15, 2024 · 10 comments
Labels
feature request New feature or request invalid This doesn't seem right

Comments

@NajamKhalil
Copy link

I am attaching the list of sample errors list here

Generated CODE:
`
always_comb begin

automatic logic [4:0] next_c = field_storage.CSR_MAC_ADID[i0].csr_mac_adid.value;

automatic logic load_next_c = '0;

if(decoded_reg_strb.CSR_MAC_ADID[i0] && decoded_req_is_wr) begin // SW write

next_c = (field_storage.CSR_MAC_ADID[i0].csr_mac_adid.value & ~decoded_wr_biten[4:0]) | (decoded_wr_data[4:0] & decoded_wr_biten[4:0]);

load_next_c = '1;

end

field_combo.CSR_MAC_ADID[i0].csr_mac_adid.next = next_c;

field_combo.CSR_MAC_ADID[i0].csr_mac_adid.load_next = load_next_c;

end`

ERRORS:

`######### BuiltIn -> RuleGroup=Design Read
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

ID Rule Alias Severity File Line Wt Message

======================================================================================

[53] SYNTH_89 SYNTH_89 SynthesisWarning /home/azeem/ares/output/gen/regs/regblock/mps_adid_sel_csr.sv 168 1000 Initial Assignment at Declaration for ( load_next_c ) is ignored by synthesis

[55] SYNTH_89 SYNTH_89 SynthesisWarning /home/azeem/ares/output/gen/regs/regblock/mps_adid_sel_csr.sv 189 1000 Initial Assignment at Declaration for ( load_next_c ) is ignored by synthesis`

@amykyta3
Copy link
Member

Can you also share which synthesis tool is reporting this issue, and which version?

In-scope initial assignments to automatic variables are synthesizable in all modern tools I have used, so this is a bit surprising.

@NajamKhalil
Copy link
Author

I am using spyglass for linting and synthesis

@amykyta3
Copy link
Member

Thanks.
Sypglass is a design analysis tool so you are likely only using it for linting and CDC. You are likely synthesizing the actual design using Synopsys's Design Compiler.

I have had very poor results from Spyglass in the past, so I am not surprised that it is flagging a warning here. Spyglass's support for even basic SystemVerilog concepts is pretty limited and often very buggy. Design Compiler uses a different analysis back-end and is generally far better. I would recommend waiving the Spyglass warning messages you shared since they are not valid in this situation (And report the issue to Solvnet, since this is a bug in Spyglass).

I have compiled similar RTL using Design Compiler, and have found no warnings or errors with this code.

Since unfortunately Spyglass is still a very popular tool in the industry, I may still change how the assignments are made to avoid the limitation in Spyglass.

Your initial question mentioned there are Error messages as well, but you only shared Warning messages. Are there additional errors you have not shared yet?

@NajamKhalil
Copy link
Author

NajamKhalil commented Mar 18, 2024

@amykyta3 the errors are related to reserved namespace.

`[5] ReserveName ERROR Error regblock/aps_csr.sv 117 10 Reserved name 'next' used

[29] ReserveName ERROR Error regblock/mps_crtbcam_csr.sv 108 10 Reserved name 'next' used

[24] ReserveName ERROR Error regblock/mps_field_offset_extractor_csr.sv 110 10 Reserved name 'next' used

[1F] ReserveName ERROR Error regblock/mps_hdoffset_extractor_csr.sv 110 10 Reserved name 'next' used

[34] ReserveName ERROR Error regblock/mps_hdr_buf0_crdts_csr.sv 104 10 Reserved name 'next' used`

@amykyta3
Copy link
Member

That is very strange. next is not a reserved keyword in the SystemVerilog language (See IEEE 1800-2017, Annex B), nor has it ever been in any of the older Verilog IEEE 1364 versions.

Is it possible that your organization is configuring Spyglass to flag additional reserved keywords beyond what is specified by the SystemVerilog LRM? According to the official language spec, next is perfectly valid as an identifier.

Also, can you share which specific version of Spyglass you are using?

@NajamKhalil
Copy link
Author

NajamKhalil commented Mar 18, 2024

SpyGlass Version : SpyGlass_vV-2023.12

@amykyta3
Copy link
Member

I see what is happening now.

Looking at the SolvNet docs, I see that the ReserveName rule is an additional rule that your organization has enabled and is configured to be an error. From the documentation on the ReserveName rule:

The ReserveName rule flags the object names that are same as the VHDL reserved words. The ReserveName rule also flags the case variants of the VHDL keywords.

It seems that your Spyglass lint policy is configured to flag SystemVerilog code that contains VHDL keywords.
Since the usage of next identifiers is generally self-contained within the generated SystemVerilog code, there is no chance of it being exposed to any external VHDL contexts. I would recommend waiving this rule violation since it is being inappropriately strict in this situation.

Since the generated SystemVerilog code is still inherently correct, I am marking this issue as invalid. Both issues reported are problems with the Spyglass tool.

@amykyta3 amykyta3 added the invalid This doesn't seem right label Mar 18, 2024
@amykyta3 amykyta3 changed the title synthesis of the PeakRDL's CSR generated RTL. Since we are seeing alot of error/warning. Spyglass issues with PeakRDL's CSR generated RTL Mar 18, 2024
@NajamKhalil
Copy link
Author

Ahan,
Apart from that, Thanks for the quick responses

@mborder-amd
Copy link

The Simulation vs. Synthesis hazard around initialized variables in ASIC design is a valid concern. Synthesis tools (Including DC/FC) will ignore the initial value of the variable, but in simulation we will see this initial assignment. Is it worth restructuring the code to something like below? This will reduce the need to carry forward waivers.

automatic logic [4:0] next_c;
automatic logic load_next_c;
next_c = field_storage.CSR_MAC_ADID[i0].csr_mac_adid.value;
load_next_c = '0;

@amykyta3
Copy link
Member

Regarding initial values, it really depends on the situation since not all initial assignments are created equal.
I agree that initial assignments to logic or reg variables that also are involved in an inference of a FF would be ignored, since there is no equivalent representation in an ASIC (ignoring FPGAs for now..)

I have seen some 3rd party ASIC IP use initial assignments to non-flop signals as a way to define constants (legacy style before localparams were widely supported).
In the PeakRDL case, the temporary variables are locally scoped automatics, and do not infer flops. They merely carry an intermediate value during a computation. In this case these are indeed synthesizable.

Nonetheless, I completely agree that a simple change would be to move the assignment to a new statement to avoid lint noise from Spyglass. Since Spyglass is still a very popular tool in the industry, I will make the change you suggested.

@amykyta3 amykyta3 added the feature request New feature or request label Mar 19, 2024
amykyta3 added a commit that referenced this issue Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request invalid This doesn't seem right
Development

No branches or pull requests

3 participants