openhwgroup / cv32e40p

CV32E40P is an in-order 4-stage RISC-V RV32IMFCXpulp CPU based on RI5CY from PULP-Platform
https://docs.openhwgroup.org/projects/cv32e40p-user-manual/en/latest
Other
936 stars 412 forks source link

Code not running with fpnew in verilator #308

Closed mmatzev closed 11 months ago

mmatzev commented 4 years ago

This is not strictly related to CV32E40P as the problem seems to be in fpnew, but did anybody verify the operation of CV32E40P with fpnew in verilator?

There is a coding-style in fpnew, which prevents verilator to work. But even if this problem is solved, the simulation is not running due to a deadlog in the div_sqrt unit. The same code runs with Questa/Modelsim without problems!?

It is hard to track the problem back to the source. I have a modelsim wlf and a verilator vcd, but with different time stamp.

Do you plan to pull fpnew into the openhwgroup repository to get cv32e40p in a reliable state also with verilator?

MikeOpenHWGroup commented 4 years ago

Hi @mmatzev, thanks for the question. I cannot answer the first part of it (do you plan to pull fpnew into an OpenHW repo), as that is currently still under discussion. I am the verification lead on CV32E40P and can answer the second part of your question.

Although the core testbench inherited from Pulp does work with Verilator, OpenHW has made no commitment to supporting Verilator in the future. The verification repository for our cores is at https://github.com/openhwgroup/core-v-verif. The testbenches in this repo are "examples" and not intended for (or capable of) complete verification of a core. We do have a core testbench in core-v-verif, but we are not planning to add capability or additional tests to it. All new verification development is taking place in the UVM environment. It is not clear to me if or when Verilator will support enough of the SystemVerilog standard to run UVM.

Hope this clarifies things.

bluewww commented 4 years ago

@mmatzev fpnew does work with verilator. Fpnew for example is used in the ariane testbench, which supports verilator. There was a branch in this repo that added the FPU to the hello world example, but never got merged https://github.com/openhwgroup/cv32e40p/commits/enable-fp-tb, I don't quite recall why.

The gist of making verilator work is the rather brutish approach of passing -Wno-BLKANDNBLK -Wno-fatal to verilator.

Verilator is not capable of running a full UVM environment so you can't expect to run the core-v-verif verification suite, but simulating just the core + fpnew RTL in your own design is not a problem at this point in time. Naturally I hope no regressions will be introduced in the RTL to make it verilator incompatible. You would really have to try hard for that, since verilator promises to support synthesizable constructs (whatever that means).

mmatzev commented 4 years ago

I discovered actually three problems with verilator, CV32e40p and fpnew. One, is self-made: I set reset to zero at the beginning of the simulation. As CV32e40p uses clock gating the clock is not passed to the registers inside of the fpu and the reset is not accepted. In verilator, the reset must be 1 at the start of simulation and than goes to 0 and back to one to work correctly. This is different to Modelsim/Questa, which can start with reset set to 0. The second is in fpu_divsqrt_multi.sv --> There are arrays of signals (outpipe), which contain flipflops and just comb e.g. out_pipe_valid_q[0] is just comb, while out_pipe_valid_q[1] is a flopflip. This does not work with verilator, but is easy to change. The third problem is that cv32e40p uses import statements with above the modules, which pollute the namespace and fpu_div_sqrt (a sub-block of fpnew) does this as well. Both define C_PC, which is therefore overwritten. The import statements should be moved into the module to avoid this.

I did all required changes and it is working, now. It is still on a quick and dirty basis and I'm not familiar with git, therefore I don't want to push it back. I can provide a patch if it is helpful for you.

bluewww commented 4 years ago

Third issue should be fixed by this https://github.com/openhwgroup/cv32e40p/issues/384

mmatzev commented 4 years ago

Yes that's my work around in CV32e40p and fpnew. I would recommend to clone fpnew also in this repository. From my point of view verilator support is very helpful for the programmers to start with an early version of the hardware.

MikeOpenHWGroup commented 4 years ago

I would recommend to clone fpnew also in this repository.

The manifest file (cv32e40p_manifest.flist) makes it clear that fpnew needs to be cloned separately.

verilator support is very helpful for the programmers to start with an early version of the hardware

Verilator is supported the the core testbench of the core-v-verif project. We have agreed to maintain a minimal amount of Verilator support as an aid to programmers.

MikeOpenHWGroup commented 4 years ago

I would recommend to clone fpnew also in this repository.

The manifest file (cv32e40p_manifest.flist) makes it clear that fpnew needs to be cloned separately.

verilator support is very helpful for the programmers to start with an early version of the hardware

Verilator is supported the the core testbench of the core-v-verif project. We have agreed to maintain a minimal amount of Verilator support as an aid to programmers.

klion717 commented 4 years ago

Hi, @MikeOpenHWGroup, I have a question about FPU status. // Status flags typedef struct packed { logic NV; // Invalid logic DZ; // Divide by zero logic OF; // Overflow logic UF; // Underflow logic NX; // Inexact } status_t; Currently, the status is only reported to the csr register. Software has to polling the csr to get the status each time there is a float point computation. So maybe an exception can generated when there occurs an Invalid/Divide by zero/Overflow/Underflow/NaN.

davideschiavone commented 4 years ago

Hello @klion717 , the FPU is RISC-V complian, which does not trigger any exception.

MikeOpenHWGroup commented 4 years ago

Hi @klion717, just so you are aware, the current iteration of the CV32E40P is not verified using the FPU. That is, all our verification is done with the top-level parameter FPU set to 0. This is described in the parameters section of the User Manual.

Silabs-ArjanB commented 3 years ago

Hi @mmatzev Of the three issues you report the first one you state is self-made, the second one relates to the fpnew repos (so not to the cv32e40p repos), and the third one should already have been fixed.

Can this issue be closed here (if needed you can file a separate issue in https://github.com/pulp-platform/fpnew of course)?

We will not clone the fpnew repos into the cv32e40p repos. In fact we are going the other direction, see https://github.com/openhwgroup/cv32e40p/pull/560.

davideschiavone commented 11 months ago

closing this issue after 3 years on inactivity. In addition, I am using the CV32E40P+FPNEW in Verilator, so this has been solved