Encountered code error when compiling cv32e40p kernel with verilator

[guojiazhuxi]

Issue Description

The problem is that in the cv32e40p/bhv/cv32e40p_sim_clock_gate.sv file, the code in the always_latch structure cannot use non -blocking assignments.

Component

Component:Verif: behavioral code

Steps to Reproduce

Please provide:

1. git hash fcd59685681bc2b58e5805497b0c59df14eda810
2. Logfile and/or wave-dump info (screen shots can be useful)

[MikeOpenHWGroup]

Hi @guojiazhuxi, thanks for your issue. I see that you are compiling the CV32E40P using the core testbench in core-v-verif. That is OK, that is what I do too! However, I do not see the errors you get. Here are my commands (note that I use GitHub's ssh authentication to clone the repo):

$ git clone git@github.com:openhwgroup/core-v-verif.git
$ cd core-v-verif/cv32e40p/sim/core
$ make sanity

The master branch of core-v-verif clones cv32e40p hash fcd5968 which is the same as you have. With the above I can compile and run using Verilator v4.100. What version of Verilator do you use?

[guojiazhuxi]

Hi, @MikeOpenHWGroup The software version I use is Verilator 5.002 2022-10-29 rev v5.002-29-gdb39d70c7 When I modify the <= in the always_latch to=, I can work normally

[MikeOpenHWGroup]

Interesting. I see that this is a latent bug in our code and it seems that up to now most simulators have ignored it.

@pascal, @davideschiavone, what do you think? The code in question is here.

[pascalgouedo]

Hi @guojiazhuxi

In IEEE Std 1800-2012 for SystemVerilog, always_latch uses non-blocking assignments.

[MikeOpenHWGroup]

Good one @pascalgouedo! Just to double check, 1800-2017 agrees.

[davideschiavone]

wooow so this is wrong everywhere I worked on! Is Verilog different ? shall we change it ?

[pascalgouedo]

I don't know. I see that cv32e40p_register_file_latch.sv is using "incorrect" blocking assignment. ETH/Unibo/Dolphin have made chips with RI5CY including a latch-based RF with this "incorrect" syntax. They went through all steps from simulation (no verilator for Dolphin) to PNR without problems and chips were fully alive and functional!!!

[pascalgouedo]

Maybe all tools are doing like stated in the verilator warning message:

... This will be executed as a blocking assignment'='!

[abukharmeh]

Corresponding issue from Verilator https://github.com/verilator/verilator/issues/864

[pascalgouedo]

Thanks @abukharmeh Seems Verilator changed their always_latch linting after this April 29, 2022 (not silent anymore?) to now have a warning for this NBA to BA conversion. And their description is quite coherent with respect to always_comb/always_latch.

So should we go to use blocking assignment in always_latch and correct cv32e40p_sim_clock_gate.sv without touching to cv32e40p_register_file_latch.sv?

[davideschiavone]

fine for me as it is not a synthesisized block. pinging also @Silabs-ArjanB for cv32e40x and s and @christian-herber-nxp for cve2

[Silabs-ArjanB]

Doesn't IEEE Std 1800-2012 simply allow what we are doing here? If so, can you give a good reason to change this? To me lack of Verilator support is not a good enough reason.

[MikeOpenHWGroup]

Doesn't IEEE Std 1800-2012 simply allow what we are doing here? If so, can you give a good reason to change this? To me lack of Verilator support is not a good enough reason.

I agree that lack of Verilator support is an insufficient reason to change. Having said that, both 1800-2012 and -2017 agree that we should not have blocking assignments in always_latch blocks. So I would argue a change to v2.0.0 of CV32E40P for that reason. It is not worth changing v1.0.0.

[Silabs-ArjanB]

Hi @MikeOpenHWGroup

I agree that lack of Verilator support is an insufficient reason to change. Having said that, both 1800-2012 and -2017 agree that we should not have blocking assignments in always_latch blocks.

Can you highlight the part of the 1800-2017 documentation where that is written?

Also at the start of this thread both @pascalgouedo and you seem to confirm the opposite (can you explain that change in opinion?):

In IEEE Std 1800-2012 for SystemVerilog, always_latch uses non-blocking assignments.

[MikeOpenHWGroup]

In 1800-2017 the relavant clause is 9.2.2.3:

SystemVerilog also provides a special always_latch procedure for modeling latched logic behavior. For example:

                         always_latch
                                if(ck) q <= d;

The always_latch construct is identical to the always_comb construct except that software tools should perform
additional checks and warn if the behavior in an always_latch construct does not represent latched logic,
whereas in an always_comb construct, tools should check and warn if the behavior does not represent
combinational logic. All statements in 9.2.2.2 shall apply to always_latch.

Representing latched logic requires a blocking assignment, so in the above q = d should generate a warning (not an error).

[Silabs-ArjanB]

@MikeOpenHWGroup

Representing latched logic requires a blocking assignment

Why do you say that?

so in the above q = d should generate a warning (not an error).

There is no 'q = d' in the above.

Also, if you state that no error should be generated it sounds like you are saying that both blocking and non-blocking is acceptable.

[MikeOpenHWGroup]

Why do you say that?

For the same reason that a BA is required to infer a flop in a clocked always block.

Also, if you state that no error should be generated it sounds like you are saying that both blocking and non-blocking is acceptable.

I am not saying anything. I'm just repeating what the LRM says (or, more accurately, my interpretation of the LRM). AFAICT, Verilator 5.002 (the version used by @guojiazhuxi) is compliant with the LRM while Verilator 4.100 (the version I have), is not. It is also worth noting that Xcelium 20.03 does not complain about this code either.

I will say that I believe the original issue report is correct:

The problem is that in the cv32e40p/bhv/cv32e40p_sim_clock_gate.sv file, the code in the always_latch structure cannot use non-blocking assignments.

So we have a few paths forward:

1. Fix it!
2. Add a compile-time waiver for Verilator.
3. Mark it wont-fix.

This is a design repo, so I will not dictate which you choose.

[Silabs-ArjanB]

Why do you say that?

For the same reason that a BA is required to infer a flop in a clocked always block.

No, a non-blocking assignment is the preferred manner to infer a flop.

Also, if you state that no error should be generated it sounds like you are saying that both blocking and non-blocking is acceptable.

I am not saying anything. I'm just repeating what the LRM says (or, more accurately, my interpretation of the LRM). AFAICT, Verilator 5.002 (the version used by @guojiazhuxi) is compliant with the LRM while Verilator 4.100 (the version I have), is not. It is also worth noting that Xcelium 20.03 does not complain about this code either.

Could you please add a screenshot of the relevant part of the SystemVerilog LRM that you use for your recommendation?

So we have a few paths forward:

1. Fix it!

2. Add a compile-time waiver for Verilator.

3. Mark it wont-fix.

I would first like to see what part of the SystemVerilog LRM is actually being violated.

[MikeOpenHWGroup]

Obviously you do not agree with my interpretation of LRM clause 9.2.2.3 That's your call.

[MikeOpenHWGroup]

Sorry, I've mixed up the terminology. My position is that the LRM requires a non-blocking-assignment (NBA). Two reasons for that:

1. The coding example in the LRM uses a NBA: always_latch if (ck) q <=d.
2. The LRM says:

The always_latch construct is identical to the always_comb construct except that software tools should perform additional checks and warn if the behavior in an always_latch construct does not represent latched logic,

The CV32E40P code that generated this issue is not a code bug, it is a Verilator bug, and I agree that we should not be modifying our code to support Verilator bugs.

Having said that, the cv32e40p_register_file_latch.sv uses a blocking assignment in an always_latch block, and I believe that is not compliant with the LRM.

[Silabs-ArjanB]

Then it seems to me we agree on this issue being a Verilator issue as opposed to an issue with cv32e40p/bhv/cv32e40p_sim_clock_gate.sv. For cv32e40p_register_file_latch.sv a separate issue could/should be opened.

[MikeOpenHWGroup]

Hi @guojiazhuxi, the consensus seems to be that there is no problem with cv32e40p/bhv/cv32e40p_sim_clock_gate.sv as originally reported in this issue. If you agree, please close this issue.

I will open a separate one for cv32e40p/rtl/cv32e40p_register_file_latch.sv.

[guojiazhuxi]

Hi @guojiazhuxi, the consensus seems to be that there is no problem with cv32e40p/bhv/cv32e40p_sim_clock_gate.sv as originally reported in this issue. If you agree, please close this issue.

I will open a separate one for cv32e40p/rtl/cv32e40p_register_file_latch.sv.

OK