openhwgroup / core-v-xif

RISC-V eXtension interface that provides a generalized framework suitable to implement custom coprocessors and ISA extensions
https://docs.openhwgroup.org/projects/openhw-group-core-v-xif/en
Other
53 stars 23 forks source link

Use of SystemVerilog Interface should be strongly discouraged #227

Open MikeOpenHWGroup opened 1 week ago

MikeOpenHWGroup commented 1 week ago

Is there an existing CV-X-IF task for this?

Task Description

This repo hosts an example SystemVerilog Interface of the CV-X-IF in src/core_v_xif.sv. It should be made clear that this is a simulation only example and should not be used in any CORE-V RTL IP.

The CV32E40P coding style guidelines, which are essentially the lowRISC coding style guidelines, indicate that SystemVerilog Interfaces are problematic constructs and their use is discouraged.

Description of Done

core_v_xif.sv is a useful example, so it is proposed that we simply add the following to the comment header of that file:

// EXAMPLE ONLY
// The lowRISC coding style guidelines (https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md)
// indicate that SystemVerilog Interfaces are problematic constructs and their use in CORE-V RTL IP is strongly discouraged.
Silabs-ArjanB commented 1 week ago

I strongly disagree with this. Use of SystemVerilog interfaces should actually be encouraged for certain uses (including this one). Removing myself from this issue.

christian-herber-nxp commented 1 week ago

i think cv-x-if should neither encourage or discourage particular coding styles are choices.

MikeOpenHWGroup commented 1 week ago

Hi @Silabs-ArjanB, can you provide a specific example of where/how SV Interfaces can be safely used in RTL? This issue was motivated by a recent discussion within the CVA6 team in which both myself and @Jbalkind expressed concerns over the use of SV Interfaces in RTL. We have both (independently) had specific issues with the EDA toolchain. In my case, a tool used for RTL-to-gates Equivalence Checking failed to properly handle the SV Interfaces. Admittedly that was several years ago. @Jbalkind, can you comment on the issue you had with SV interfaces?

I will reiterate that the OpenHW Cores Task Group adopted the lowRISC coding style guidelines which indicate that SystemVerilog Interfaces are problematic constructs and their use is discouraged.

Silabs-ArjanB commented 1 week ago

Hi @MikeOpenHWGroup

can you provide a specific example of where/how SV Interfaces can be safely used in RTL?

https://github.com/openhwgroup/cv32e40x/blob/master/rtl/cv32e40x_core.sv

I will reiterate that the OpenHW Cores Task Group adopted the [lowRISC coding style guidelines]

We treat the lowRISC coding style guidelines as exactly that, i.e. guidelines. Mostly these are good guidelines, but we don't take them too literally. They were only adopted because nobody wanted to spend time/effort in writing our own guidelines and/or opening up internal guidelines.

davideschiavone commented 1 week ago

I agree that the use of the SV interfaces should neither be encouraged nor discouraged and that the lowRISC coding style is a guideline, but not a strict rule.

There are many reasons why SV interfaces are the preferred choice, X-IF would be one example (although can be done with structures as well)

Jbalkind commented 6 days ago

I have to say that I disagree with the suggestion we should be embracing interfaces. When this was discussed in the CVA6 meeting, there were multiple responses from OpenHW members saying that they had designs which were broken by the use of interfaces. One member has a chip which does not function correctly because Genus didn't handle interfaces correctly. We should not be distributing code which has known problems when used with well known tools.

I like interfaces and I wish that they were better supported but I do not think we should be including them in our codebases until we are past the point that they are a common concern. If we are giving types using interfaces then we should also be giving associated structs as examples to avoid what has already happened (contributors wasting their time to use interfaces when their use shouldn't be merged).