The-OpenROAD-Project / OpenROAD-flow-scripts

OpenROAD's scripts implementing an RTL-to-GDS Flow. Documentation at https://openroad-flow-scripts.readthedocs.io/en/latest/
https://theopenroadproject.org/
Other
313 stars 275 forks source link

Question: Are there any examples that use custom cell mappings? (cells_latch.v, cells_adder.v, cells_clkgate.v) #613

Closed stevenmburns closed 1 year ago

stevenmburns commented 1 year ago

Is your feature request related to a problem? Please describe. I'm trying to create the correct platform collateral for intel16. I have filled in good candidate cells for the active high and active low transparent latches, good half and full adders, and an integrated clock gating cell. Is there an existing way to test whether these new cells are being used? I have cases where I can see the adder and clock gate being used, but I'm not sure how to specify a latch in verilog so that yosys will understand it. Any pointers?

stevenmburns commented 1 year ago

Here is an attempt to use latches:

module latch_based_reg_file(clk, wdata, we, waddr, raddr, rdata);
   input clk;

   input [2:0] wdata;
   input       we;
   input [0:0] waddr;
   input [0:0] raddr;

   output [2:0] rdata;

   reg [2:0] rdata;

   wire   [2:0] latched_wdata;

   wire raw_en_clk_0;
   wire raw_en_clk_1;

   wire en_clk_0;
   wire en_clk_1;

   wire [2:0]   state_0;
   wire [2:0]   state_1;

   assign raw_en_clk_0 = we & (waddr == 1'b0);
   assign raw_en_clk_1 = we & (waddr == 1'b1);

   $_DLATCH_N_ L_0 (.E(clk), .D(wdata[0]), .Q(latched_wdata[0]));
   $_DLATCH_N_ L_1 (.E(clk), .D(wdata[1]), .Q(latched_wdata[1]));
   $_DLATCH_N_ L_2 (.E(clk), .D(wdata[2]), .Q(latched_wdata[2]));

   OPENROAD_CLKGATE CG_0 (.CK(clk), .E(raw_en_clk_0), .GCK(en_clk_0));
   OPENROAD_CLKGATE CG_1 (.CK(clk), .E(raw_en_clk_1), .GCK(en_clk_1));

   $_DLATCH_P_ D_0_0 (.E(en_clk_0), .D(latched_wdata[0]), .Q(state_0[0]));
   $_DLATCH_P_ D_0_1 (.E(en_clk_0), .D(latched_wdata[1]), .Q(state_0[1]));
   $_DLATCH_P_ D_0_2 (.E(en_clk_0), .D(latched_wdata[2]), .Q(state_0[2]));

   $_DLATCH_P_ D_1_0 (.E(en_clk_1), .D(latched_wdata[0]), .Q(state_1[0]));
   $_DLATCH_P_ D_1_1 (.E(en_clk_1), .D(latched_wdata[1]), .Q(state_1[1]));
   $_DLATCH_P_ D_1_2 (.E(en_clk_1), .D(latched_wdata[2]), .Q(state_1[2]));

   always_comb begin
      rdata = 3'bx;
      if (raddr == 1'b0) begin
         rdata = state_0;
      end else if (raddr == 1'b1) begin
         rdata = state_1;
      end
   end

endmodule

This fails with:

ERROR: Module `\$_DLATCH_P_' referenced in module `\latch_based_reg_file' in cell `\D_1_2' is not part of the design.
maliberty commented 1 year ago

I'm not aware of any designs in our suite using latches. @louiic @rovinski @ravi-varadarajan do you know of any?

rovinski commented 1 year ago

We generally try to avoid using latches in designs. This is a special use case where it's appropriate.

@stevenmburns could you package a full test case?

stevenmburns commented 1 year ago

latch_design.tar.gz

I ported the testcase to nangate45. Here is the error, not recognizing the LATCH primitive, which I don't know how to specify correctly:

Apptainer> make DESIGN_CONFIG=./designs/nangate45/latch_based_reg_file/config.mk 
[INFO][FLOW] Using platform directory ./platforms/nangate45
./util/markDontUse.py -p "TAPCELL_X1 FILLCELL_X1 AOI211_X1 OAI211_X1" -i platforms/nangate45/lib/NangateOpenCellLibrary_typical.lib -o objects/nangate45/latch_based_reg_file/base/lib/NangateOpenCellLibrary_typical.lib
Opening file for replace: platforms/nangate45/lib/NangateOpenCellLibrary_typical.lib
Marked 4 cells as dont_use
Commented 0 lines containing "original_pin"
Replaced malformed functions 0
Writing replaced file: objects/nangate45/latch_based_reg_file/base/lib/NangateOpenCellLibrary_typical.lib
mkdir -p ./results/nangate45/latch_based_reg_file/base ./logs/nangate45/latch_based_reg_file/base ./reports/nangate45/latch_based_reg_file/base
(/usr/bin/time -f 'Elapsed time: %E[h:]min:sec. CPU time: user %U sys %S (%P). Peak memory: %MKB.' /OpenROAD-flow-scripts/tools/install/yosys/bin/yosys -v 3 -c ./scripts/synth.tcl) 2>&1 | tee ./logs/nangate45/latch_based_reg_file/base/1_1_yosys.log
1. Executing Verilog-2005 frontend: ./designs/src/latch_based_reg_file/latch_based_reg_file.v
2. Executing Liberty frontend.
3. Executing Verilog-2005 frontend: ./platforms/nangate45/cells_clkgate.v
4. Executing SYNTH pass.
4.1. Executing HIERARCHY pass (managing design hierarchy).
4.2. Executing AST frontend in derive mode using pre-parsed AST for module `\latch_based_reg_file'.
4.2.1. Analyzing design hierarchy..
ERROR: Module `\$_DLATCH_P_' referenced in module `\latch_based_reg_file' in cell `\D_1_2' is not part of the design.
Elapsed time: 0:00.15[h:]min:sec. CPU time: user 0.13 sys 0.01 (98%). Peak memory: 30896KB.
make: *** [results/nangate45/latch_based_reg_file/base/1_1_yosys.v] Error 1
stevenmburns commented 1 year ago

I'm not that interested in latch based designs, I just want to make sure that I have the correct collateral for the new platform I'm trying to setup. nangate45 specifies cell mappings for latches, full_adder/half_adder, and integrated clock gates, in cells_latch.v, cells_adder.v, and cells_clkgate.v. I want to do the same, but I also want to be able to test that what I did is being consumed correctly. If nobody uses latches, then maybe this isn't important and it should be dropped from all libraries.

rovinski commented 1 year ago

I don't think cells_latch.v, cells_adder.v, or cells_clkgate.v are required unless those cells are explicitly instantiated in the design. cells_adder.v is definitely useful because you can't infer half or full adder cells unless it's specified - yosys will use generic logic instead. cells_latch.v and cells_clkgate.v won't be used unless the design calls for it. Generally, latches aren't good in pure digital designs and our benchmarks don't test them, as far as I know. You'll find that you can make a highly functional platform setup without those files and you will also be able to run the entire benchmark suite without them.

All that being said, we should definitely make sure we support them because they are important in certain use cases such as this.

stevenmburns commented 1 year ago

To make this design work, I probably only need to know how to make instances of the yosys primitives $_DLATCH_P_ and $_DLATCH_N_.

maliberty commented 1 year ago

You don't directly instantiate the latches but give synthesis input that it maps to latches. For example:

module latchinf (E, D, Q);
   input  E, D;
   output Q;
   reg    Q;
   always @ (E or D)
      if (E)
         Q <= D;
endmodule

module latch_based_reg_file(clk, wdata, latched_wdata);
   input clk;
   input [2:0] wdata;
   output   [2:0] latched_wdata;

   latchinf L_0 (.E(clk), .D(wdata[0]), .Q(latched_wdata[0]));
   latchinf L_1 (.E(clk), .D(wdata[1]), .Q(latched_wdata[1]));
   latchinf L_2 (.E(clk), .D(wdata[2]), .Q(latched_wdata[2]));

endmodule

gives instances of DLH_X1 in the yosys output.

stevenmburns commented 1 year ago

Thanks, that works. (I tried that originally but I must have done something else wrong because it was propagating 1'b0 to the gated clocks.) The one remaining thing is what is the preferred way to make instances of OPENROAD_CLKGATE create a clock gate.

   OPENROAD_CLKGATE CG_0 (.CK(clk), .E(raw_en_clk_0), .GCK(en_clk_0));

I've instantiated it above. It doesn't get converted to a clock gate in the nangate45 platform because the ifdef isn't satisfied in the definition below. Are we suppose to define that somewhere. If so, where?

Apptainer> cat platforms/nangate45/cells_clkgate.v
module OPENROAD_CLKGATE (CK, E, GCK);
  input CK;
  input E;
  output GCK;

`ifdef OPENROAD_CLKGATE

CLKGATE_X1 latch (.CK (CK), .E(E), .GCK(GCK));

`else

assign GCK = CK;

`endif
maliberty commented 1 year ago

@tajayi the only usage I see for OPENROAD_CLKGATE is in designs/src/swerv/swerv_wrapper.sv2v.v which you setup. Do you know the history here?

rovinski commented 1 year ago

I think swerv_wrapper uses an architectural clock gate so that was the best strategy for making it portable. I don't know whether yosys supports inferring clock gates from RTL (I don't think it does).

I think there might be something lost along the way because OPENROAD_CLKGATE doesn't get defined anywhere in the synth script, so this would always result in evaluating to assign GCK = CK.

maliberty commented 1 year ago

The naming style doesn't match what yosys uses so this seems like an ORFS hack from what I can see. I've tried asking about clock gate inferencing in the yosys slack as I didn't see anything in the manual. In short I wouldn't worry about this case for now.

maliberty commented 1 year ago

I got confirmation that yosys doesn't generate clock gating so this is not a general feature.

@rovinski is there any reason not to just turn this on in this design and remove it from the scripts?

rovinski commented 1 year ago

@rovinski is there any reason not to just turn this on in this design and remove it from the scripts?

Do you mean add the clock gate to this design and then remove it from the platforms? IDK I think 1) it sets a good example for other designs 2) It isn't impacting the flow, but otherwise yeah it could be removed.

maliberty commented 1 year ago

On the flip side it is confusing to anyone bringing up a new PDK as this issue shows. It makes it seem like a yosys feature which it isn't. I don't feel strongly one way or the other.

rovinski commented 1 year ago

Another option is to add documentation about it. I could make a quick PR.

stevenmburns commented 1 year ago

My recommendation would be to get rid of the ifdef here.

Apptainer> cat platforms/nangate45/cells_clkgate.v
module OPENROAD_CLKGATE (CK, E, GCK);
  input CK;
  input E;
  output GCK;

`ifdef OPENROAD_CLKGATE

CLKGATE_X1 latch (.CK (CK), .E(E), .GCK(GCK));

`else

assign GCK = CK;

`endif
endmodule

It is not the same thing to just pass the clock through anyway. How about this instead?

Apptainer> cat platforms/nangate45/cells_clkgate.v
module OPENROAD_CLKGATE (CK, E, GCK);
  input CK;
  input E;
  output GCK;
CLKGATE_X1 latch (.CK (CK), .E(E), .GCK(GCK));
endmodule

Then new platforms could add this or not, but it will be used if the testcase has it.

Feel free to close this issue (or I will if you want.)