Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dangerous use of blackboxes #1291

Open
armleo opened this issue Aug 28, 2022 · 8 comments
Open

Dangerous use of blackboxes #1291

armleo opened this issue Aug 28, 2022 · 8 comments
Labels
enhancement New feature or request OpenLane 2 Scheduled for next gen OpenLane

Comments

@armleo
Copy link
Contributor

armleo commented Aug 28, 2022

Prompt

The flow needs proper way to handle the blackbox cells.

Verilog blackbox is used by the synthesis tool.
It tells the synthesis tool the purpose and width of the Input and Output.
In the future versions this should be fixed by generating liberty files and loading it in the top level macro blocks.
Meanwhile, the users should be careful when making sub components that have parameter, because this may cause missmatches between RTL and the final GDS.

Create the verilog blackbox:

    (*blackbox*)

    module mem_1r1w (clk, read_addr, read, read_data, write_addr, write, write_data);
        parameter DEPTH_LOG2 = 4;
        localparam ELEMENTS = 2**DEPTH_LOG2;
        parameter WIDTH = 32;

        parameter CRITICAL_BEHAVOURAL_PARAMETER=1;

        input wire clk;

        input wire [DEPTH_LOG2-1:0] read_addr;
        input wire read;
        output reg [WIDTH-1:0] read_data;


        input wire [DEPTH_LOG2-1:0] write_addr;
        input wire write;
        input wire  [WIDTH-1:0] write_data;

    endmodule

Then in the code it is referenced twice:
mem_1r1w (CRITICAL_BEHAVOURAL_PARAMETER = 0) vs mem_1r1w (CRITICAL_BEHAVOURAL_PARAMETER = 1).

Note the usage of CRITICAL_BEHAVOURAL_PARAMETER. This parameter changes the behavoural of the device.

Down the line we replace mem_1r1w (CRITICAL_BEHAVOURAL_PARAMETER = 0) with an RTL-to-GDS output. Now the second mem_1r1w (CRITICAL_BEHAVOURAL_PARAMETER = 1) references the output of RTL-to-GDS. This is dangerous.

Proposal

Proper support for blackboxes, SDF writing and loading by the flow, blackbox .lib support, etc.

@kareefardi
Copy link
Collaborator

Are you trying to say that verilog parameters have no effect in blackbox modules ?

@armleo
Copy link
Contributor Author

armleo commented Aug 28, 2022

Yes. There is no check to make sure that the RTL used for RTL-to-GDS flow for the underlying cell matched the parameter-wise referenced cell. There is no check for this. It would be unreasonable for this check to exist. But this particular issue needs to be fixed, regardless.

I do not know, how to explain it, quite honestly. I can explain on Thursday meeting.

@donn donn added enhancement New feature or request Flow Script labels Aug 29, 2022
@kareefardi
Copy link
Collaborator

kareefardi commented Aug 29, 2022

I think I understand. The thing about blackboxed macros is that, well, they are blackboxed. So I don't it is possible with the current tools at hand to have "parameterized" blackboxed macros, if such thing exist. But I do feel you concern about someone missing that. I think there is a way were yosys generates different names for a macro based on its parameters which I think can be leveraged here, again if there is really such a thing. and in that case the user shall be required to provide the macro as two or more macros independently. Otherwise, the best effort that can be done is detecting the usage of a macro multiple times and warn that an error with parameters is possible.

@donn
Copy link
Collaborator

donn commented Aug 29, 2022

Yeah, I'm gonna go on record- if a macro's provided with a blackbox Verilog model that's parameterized, that macro has a bug that needs fixing, and OpenLane should probably check and enforce that.

Like, not checking whether the user is using parameters: Macros should not have parameters, period. They're hardened. Whatever they're hardened with is whatever exists.

@kareefardi
Copy link
Collaborator

That's also a possibility.

@maliberty
Copy link
Member

You should not black box mem_1r1w but a specific instantiation like mem_1r1w (CRITICAL_BEHAVOURAL_PARAMETER = 0).

@armleo
Copy link
Contributor Author

armleo commented Aug 29, 2022

@maliberty

I am fully aware of that. I intentionally crafted this example to show the issues that I had while writing the guide for blackboxing/integrating sub macros.

But some users may miss this. We need .lib support with proper P/G handling to make sure that end users do not end up with this issue.

@armleo
Copy link
Contributor Author

armleo commented Aug 30, 2022

P.S: The blackboxes in other tools can't have parameters. They produce an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request OpenLane 2 Scheduled for next gen OpenLane
Projects
None yet
Development

No branches or pull requests

4 participants