SystemRDL / PeakRDL-regblock

Generate SystemVerilog RTL that implements a register block from compiled SystemRDL input.
http://peakrdl-regblock.readthedocs.io
GNU General Public License v3.0
47 stars 29 forks source link

Spyglass issues with PeakRDL's CSR generated RTL #87

Closed NajamKhalil closed 3 months ago

NajamKhalil commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

I am using spyglass for linting and synthesis

amykyta3 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

SpyGlass Version : SpyGlass_vV-2023.12

amykyta3 commented 3 months ago

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.

NajamKhalil commented 3 months ago

Ahan, Apart from that, Thanks for the quick responses

mborder-amd commented 3 months ago

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 commented 3 months ago

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.