lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
345 stars 122 forks source link

Use of SystemVerilog Interfaces #52

Open MikeOpenHWGroup opened 3 years ago

MikeOpenHWGroup commented 3 years ago

This guide lists SV interfaces as problematic and that their use is discouraged. Do you have a specific rationale for this?

My own experience is that SV Interfaces are invaluable for verification code, and can be used for RTL as long as care is taken with synthesis. I do not know if SV Interfaces are supported by all FPGA synthesis tools (its been a while since I've tried that).

imphil commented 3 years ago

The style guide at https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md is primarily focused on synthesizeable SV, which passes through a much larger set of tools than DV code, leading to a rather conservative choice of allowed features. We (me personally and others on OpenTitan) had various cases in the past where interfaces were mis-synthesized, partially supported, etc.

Unfortunately I don't have specific test cases or specific bugs at hand to be able to check if that is still a valid concern, making it effectively a risk trade-off: do interfaces provide enough value to try again? For OpenTitan, we didn't find a good reason to use them again, but we'd be very interested in the experience others have!

@sjgitty @tjaychen @eunchan do you remember more details of when you last tried SV interfaces? (I think Scott had a story there about a misinterpretation of some code by a tool very late in the design flow, leading to a hard-to-diagnose bug shortly before tapeout.)

For DV code, SV interfaces are usable and widely used within OpenTitan and Ibex DV, even though we don't explicitly allow them in the DV style guide at https://github.com/lowRISC/style-guides/blob/master/DVCodingStyle.md#interfaces-clocking-blocks-modports.

@sriyerg that's something we should probably spell out more strongly?

sriyerg commented 3 years ago

We already maximize code-reuse and reduce potential connectivity mistakes by bundling common port signals into structs. I would assume the historic decision to prefer structs over interfaces was to ensure support for a wide range of tools, including some of the open source ones. Perhaps someone can comment on whether there were tools that did not play nice with interfaces.

In general I'd imagine the RTL designer-maintained interfaces will come with limitations (confined to synthesizable subset of SV, for starters), so that weakens the argument of being able to reuse them in DV code. For DV, implementing interfaces is crucial - its the most flexible way to get the dynamic side of SV (OOP / classes-based verification) to talk to the static side (RTL, tb). I'd rather prefer to let the DV have the freedom to implement interfaces the way they see fit (add tasks, functions, assertions, other verification logic etc.).

I don't see a reason to explicitly state that in the DV style guide because its a given (it does not explicitly disallow them either). Though, I am happy to update it if you think it is necessary.

MikeOpenHWGroup commented 3 years ago

Thanks for the comments everyone. To @sriyerg's comment:

I don't see a reason to explicitly state that in the DV style guide...

I certainty agree with that (and everything else you said for that matter). My comment/question was about the RTL style guide (VerilogCodingStyle), not the DV style guide. Sorry that I was not clear on that point.

You can close this issue if you wish.

eunchan commented 3 years ago

As far as I know, DC, Primetime did not have issues on the interface usage. But the interface should be really basic, mostly bundling the signal only (no clocking). So, at the time, we decided to go for a safer route.

sriyerg commented 3 years ago

We should update the RTL style guide then to clearly explain why prefer structs over interfaces in that case.

tjaychen commented 3 years ago

this is one of the reasons (but may have been addressed). Interfaces, at least in the past, did not play well with power domain boundaries.

So the tools would frequently get confused about whether isolation cells should be placed in both physical and simulation functions. This may have gotten a lot better since, the last time I talked to others about this was roughly in 2018, and it sounded like the safer bet back then was still structs and not interfaces.

On Wed, May 5, 2021 at 12:30 PM Srikrishna Iyer @.***> wrote:

We should update the RTL style guide then to clearly explain why prefer structs over interfaces in that case.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/style-guides/issues/52#issuecomment-832951810, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSUSR64UM5VQZB2OFNDTMGMD7ANCNFSM44DGYSIQ .

josuah commented 1 year ago

My gratitude to @tjaychen for offering an explanation.

If the name of that tool can be disclosed, we could ask publicly (open-source toolchain) or in private (part of a proprietary toolchain) and report the result here.

josuah commented 1 year ago

Eventually several aspects of interfaces could be checked to see if an useful subset is still supported, or none at all.

I am happy to try that list with tools you had doubt with.

tjaychen commented 1 year ago

My gratitude to @tjaychen for offering an explanation.

If the name of that tool can be disclosed, we could ask publicly (open-source toolchain) or in private (part of a proprietary toolchain) and report the result here.

sorry i'm really not sure if i can say it in the open. The project that experienced these issues is itself not an open source project. However I think we could reach out to the major tool vendors and just ask in general how well interfaces are supported now. Being able to use multi-directional interfaces is IMO cleaner than singular direction structs.

josuah commented 1 year ago

It is good that this Style Guide also gets tested with the proprietary tools, which could help compatibility across vendors, but also between vendors and open source toolchains.

I did just try today with what I had access to (verilator and yosys), and both support enough of SystemVerilog interfaces for bundling multiple input/output signals together.

As someone who is learning, projects like this, as well as all the code to read from, offer much insight to me. Thank you all.

josuah commented 1 year ago

While simulation with Verilator went without trouble, I could not yet find a way to use interfaces that did play well with yosys.

It might be worth having a look at progress of issues such as https://github.com/YosysHQ/yosys/issues/1053 or https://github.com/YosysHQ/yosys/issues/1592 when considering interfaces in a project.

josuah commented 1 year ago

Someone else could test the below on Lattice and Synplify synthesis engines which are part of Radiant and it worked well:

interface iBus (
    input logic clk,
    input logic rst
);
    logic req;

    modport mpController (
        input   clk,
        input   rst,
        output  req
    );

    modport mpPeripheral (
        input   clk,
        input   rst,
        input   req
    );
endinterface

module mLighter (
    iBus.mpPeripheral  bus,
    output logic [7:0] led
);
    always_ff @(posedge bus.clk, posedge bus.rst)
        if (bus.rst)
            led <= 0;
        else if (bus.req) begin
            led <= led + 'd1;
        end
endmodule

module mTopLevel (
    input  logic clk,
    input  logic rst,
    input  logic req,
    output logic [7:0] led
);
    // instantiate the bus interface
    iBus bus (.clk(clk), .rst(rst));

    // issue a request over the bus
    always_ff @(posedge clk, posedge rst)
        if (rst) bus.req <= 0;
        else     bus.req <= req;

    // instantiate the module
    mLighter peripheral0 (
        .bus(bus.mpPeripheral),
        .led(led             )
    );
endmodule

unknown

This lets us yosys, icarus verilog, and possibly other vendor tools that lack a complete enough support for them in synthesis. And Verilator, Radiant, and possibly other vendor tools that have enough coverage for a fully working synthesis.

P.S.: sorry for the non-conformant indentation, I am in the process of converting my sources.