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

Generate primitives with multiple aspect ratios and then use the placer to decide which to choose. #671

Merged
merged 32 commits into from
Apr 30, 2021

Conversation

stevenmburns
Copy link
Collaborator

@stevenmburns stevenmburns commented Apr 27, 2021

- [x] Set up a simple test framework where we hack in extra primitives and make the placer work

  • Hack generation of multiple primitives with equal total 'nfin'
  • Modify verilog json and placement verilog json to use "abstract_template_name" and "concrete_template_name"
  • Create C++ data structure to represent the abstract to concrete map. Should be a string to vector[string] map. (Replaces string to string map.)
  • Use the new map with the placer.

@stevenmburns
Copy link
Collaborator Author

stevenmburns commented Apr 27, 2021

@kkunal1408 @parijatm @soneryaldiz @arvuce22 @854768750 @Lastdayends
How do we decide the current aspect ratio of the primitives?
Currently only one the last of these below is chosen for a nfin240.
Is there any reason we can't generate all four and let the placer choose the best one?
The product of n, X, Y should be 240 in each case.

  "Switch_PMOS_nfin240_n10_X4_Y6_ST2_RVT": {
    "primitive": "Switch_PMOS",
    "value": 10,
    "x_cells": 4,
    "y_cells": 6,
    "parameters": {
      "w": 2.7e-08,
      "l": 2e-08,
      "nfin": 240.0,
      "stack": 2,
      "real_inst_type": "pmos_rvt"
    },
    "stack": 2,
    "vt_type": "RVT"
  },
  "Switch_PMOS_nfin240_n10_X6_Y4_ST2_RVT": {
    "primitive": "Switch_PMOS",
    "value": 10,
    "x_cells": 6,
    "y_cells": 4,
    "parameters": {
      "w": 2.7e-08,
      "l": 2e-08,
      "nfin": 240.0,
      "stack": 2,
      "real_inst_type": "pmos_rvt"
    },
    "stack": 2,
    "vt_type": "RVT"
  },
  "Switch_PMOS_nfin240_n12_X4_Y5_ST2_RVT": {
    "primitive": "Switch_PMOS",
    "value": 12,
    "x_cells": 4,
    "y_cells": 5,
    "parameters": {
      "w": 2.7e-08,
      "l": 2e-08,
      "nfin": 240.0,
      "stack": 2,
      "real_inst_type": "pmos_rvt"
    },
    "stack": 2,
    "vt_type": "RVT"
  },
  "Switch_PMOS_nfin240_n12_X5_Y4_ST2_RVT": {
    "primitive": "Switch_PMOS",
    "value": 12,
    "x_cells": 5,
    "y_cells": 4,
    "parameters": {
      "w": 2.7e-08,
      "l": 2e-08,
      "nfin": 240.0,
      "stack": 2,
      "real_inst_type": "pmos_rvt"
    },
    "stack": 2,
    "vt_type": "RVT"
  },

If this is okay, I'm going to change the interface data structure a little bit (at the align.main level, creating both a mapping file and a primitive file) and then ask various of you to move the adapters into the appropriate modules.

@stevenmburns stevenmburns requested review from soneryaldiz and removed request for soneryaldiz April 27, 2021 19:38
@soneryaldiz
Copy link
Collaborator

soneryaldiz commented Apr 27, 2021

@stevenmburns , let's chat about this in our 1:1. The interface between the core and pdk has to change anyway to accommodate the generate constraint. Except few corner cases, what transistor array generator really needs is the number of rows to place the active array on. Given w/lnfin/nf/m from the netlist, no other parameters are needed.

We should really retire or move any custom hacks to netlist preprocessing. It might be good to allow a PDK to register a netlist preprocessor.

@854768750
Copy link
Collaborator

I can add something in PnR when multiple aspect ratio is generated.

@854768750
Copy link
Collaborator

854768750 commented Apr 28, 2021

For PnR, multiple aspect ratio needs the modification of input files:
map file:

primitive1 primitive1_x1_y2.gds
primitive1 primitive1_x2_y1.gds

lef file:

MACRO primitive1_x1_y2
MACRO primitive1_x2_y1

verilog file:

"template_name":"primitive1"
"instance_name": "m1"

@stevenmburns
Copy link
Collaborator Author

@parijatm I'm now proposing that we modify the master name fields in the placement.verilog.json after placement and we have choose concrete layouts for each of the leaf cells. This PR is doing that now for capacitors.
Whether or not two instance of the same (abstract) template name have the same layout is upto whatever constraints are provided to the system and it will need to be verified using the checker system.

Since this is different semantics than usual, we might want to replace the template_name field with two new fields: abstract_template_name and concrete_template_name. For each abstract_template_name we have a list of possible concrete_template_name strings, which the placer can chose to use (based on area, meeting constraints, etc.)

This is what the current map file is meant to do, but that functionality has slowly died (not it is always a one-to-one mapping.)

Also, we don't current write out the placement_verilog_json anywhere

@parijatm
Copy link
Collaborator

@parijatm I'm now proposing that we modify the master name fields in the placement.verilog.json after placement and we have choose concrete layouts for each of the leaf cells. This PR is doing that now for capacitors.
Whether or not two instance of the same (abstract) template name have the same layout is upto whatever constraints are provided to the system and it will need to be verified using the checker system.

Since this is different semantics than usual, we might want to replace the template_name field with two new fields: abstract_template_name and concrete_template_name. For each abstract_template_name we have a list of possible concrete_template_name strings, which the placer can chose to use (based on area, meeting constraints, etc.)

This is what the current map file is meant to do, but that functionality has slowly died (not it is always a one-to-one mapping.)

Also, we don't current write out the placement_verilog_json anywhere

All the proposed changes sound good to me. Note that we haven't yet formalized any constraints that force two primitive instances of the same (abstract) template name to have the same layout. SymmetricBlocks comes to mind but I am yet to review exactly what PnR does for these.

I am not quite clear where the list of possible concrete_template_name strings are specified though. Are we fixing the map file, looking at the primitive key in primitive.json? I would rather keep it together with the heirarchy information (.verilog.json) somehow so that we can eventually eliminate the file based transfer of information we are doing now.

@stevenmburns
Copy link
Collaborator Author

Right now I'm generate more options by modifying the entries in the original primitives.json (from the topology generator), for example by switching the X and Y values (for values that are different) and running that through the primitive generation flow. This should be the job of the topology generator (or at least it is generating the one size now) or a new flow stage that decides what different aspect ratio might make sense.

@stevenmburns
Copy link
Collaborator Author

It seems that we could add the information in primitives.json to our verilog_json file for communication between stages.

@stevenmburns
Copy link
Collaborator Author

@854768750 This is ready to rewrote the PnR code for the new map file.
Currently it looks like this (for current_mirror_ota) [I've added some more primitives]

DCL_PMOS_nfin60_RVT DCL_PMOS_nfin60_n12_X5_Y1_RVT.gds
Switch_PMOS_nfin240_ST2_RVT Switch_PMOS_nfin240_n12_X5_Y4_ST2_RVT.gds
SCM_NMOS_nfin10_RVT SCM_NMOS_nfin10_n12_X1_Y1_RVT.gds
SCM_NMOS_nfin24_RVT SCM_NMOS_nfin24_n12_X2_Y1_RVT.gds
DP_NMOS_B_nfin28_RVT DP_NMOS_B_nfin28_n12_X3_Y1_RVT.gds
DCL_PMOS_nfin60_RVT DCL_PMOS_nfin60_n12_X1_Y5_RVT.gds
Switch_PMOS_nfin240_ST2_RVT Switch_PMOS_nfin240_n12_X4_Y5_ST2_RVT.gds
SCM_NMOS_nfin24_RVT SCM_NMOS_nfin24_n12_X1_Y2_RVT.gds
DP_NMOS_B_nfin28_RVT DP_NMOS_B_nfin28_n12_X1_Y3_RVT.gds

It is crashing with some LEF fetching errors --- I think this is to be expected until we use the information in the map file correctly.

nR.PnRDB.PnRdatabase._ReadLEF INFO : PnRDB-Info: reading LEF file <string>
align.pnr.build_pnr_model INFO : Finished reading contraint json file current_mirror_ota.pnr.const.json
PnR.PnRDB.PnRdatabase.MergeLEFMapData INFO : merge LEF/map data on node current_mirror_ota
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: DCL_PMOS_nfin60_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: Switch_PMOS_nfin240_ST2_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: DCL_PMOS_nfin60_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: Switch_PMOS_nfin240_ST2_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: SCM_NMOS_nfin10_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: SCM_NMOS_nfin24_RVT
PnR.PnRDB.PnRdatabase.MergeLEFMapData ERROR : The key does not exist in map: DP_NMOS_B_nfin28_RVT
align.pnr.toplevel INFO : Starting bottom-up placement on current_mirror_ota 0
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m21
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m20s
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m19
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m18s
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m14_m16
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m11_m10
PnR.cap_placer.Placer_Router_Cap.Common_centroid_capacitor_aspect_ratio INFO : CC Capacitor Placement and Router : current_mirror_ota m17_m15
Segmentation fault

@854768750
Copy link
Collaborator

I see. I will do some changes to the PnR code.

@@ -82,7 +82,8 @@ class PnRdatabase
int unitScale;
map<string, vector<PnRDB::lefMacro> > lefData; //map from Macro name to Macro Instance
public:
map<string, string> gdsData; //map from gds name to gds file
map<string, string> gdsData; //map from gds name to gds file (won't need this one after being replaced by gdsData2)
map<string, vector<string> > gdsData2; //map from gds name to multiple gds file (abstract to multiple concrete)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@854768750 I added this as the C++ data structure of the new mapping.

with (d / mapname).open( "rt") as fp:
for line in fp:
line = line.rstrip('\n')
m = p.match(line)
assert m
k, v = m.groups()
tbl[k] = str(d / v)
return tbl
tbl2[k].append( str(d / v))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@854768750 Here tbl2 is the python version of the map from abstract name to list of concrete names.

@@ -189,7 +143,11 @@ def PnRdatabase( path, topcell, vname, lefname, mapname, drname):
DB.ReadPDKJSON( path + '/' + drname)

_ReadLEF( DB, path, lefname)
DB.gdsData = _ReadMap( path, mapname)
DB.gdsData, DB.gdsData2 = _ReadMap( path, mapname)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@854768750 Here is were the Python map get copied to the C++ map. (and the printed out so we can see it is doing the right thing.)

@stevenmburns stevenmburns changed the title Generate primitives with multiple aspect ratios and then use the placer to decide which to choose. WIP: Generate primitives with multiple aspect ratios and then use the placer to decide which to choose. Apr 29, 2021
@stevenmburns
Copy link
Collaborator Author

@854768750 I added some more stuff that you should feel free to ignore. It might be helpful if you haven't started, but probably will not be helpful if you have.

@854768750
Copy link
Collaborator

ok, I am solving the merge conflicts.

@854768750
Copy link
Collaborator

ok, then capacitors are like other primitives, multiple aspect ratio of capacitor is before PnR

@stevenmburns
Copy link
Collaborator Author

That is what we want eventually. I wasn't going to move the capacitors to the primitive stage in this PR, but soon.

@854768750
Copy link
Collaborator

ok, I will do some changes of name mapping in capplacer

@stevenmburns
Copy link
Collaborator Author

I did something simple. Ignore this check in if you have something better.

@stevenmburns
Copy link
Collaborator Author

@854768750 That seemed to fix things up well. Maybe we can wait on more fixes until another PR.

const string abstract_template_name = node.Blocks[i].instance.front().master;

if (gdsData2.find(abstract_template_name) == gdsData2.end()) {
if (abstract_template_name.find("Cap") != std::string::npos || abstract_template_name.find("cap") != std::string::npos || !node.Blocks[i].instance.back().isLeaf) continue;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@854768750 I just fir checked if the abstract_template_name was in gdsData2. If not, I'll skip the whole process if cap is in the name.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this does not cause error on capacitor because capplacer is not functioning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine because, seems in BUFFER_VREFP1, Dcap is just like other ordinary primitives.

@stevenmburns
Copy link
Collaborator Author

stevenmburns commented Apr 30, 2021

I ran the regression through and there are some other corner cases to deal with. There are three that used to work that don't anymore: powertrain_binary, test_vga, and Gm1_v5_Practice

I'm looking to see if they have anything in common.

@parijatm
Copy link
Collaborator

parijatm commented Apr 30, 2021

I ran the regression through and there are some other corner cases to deal with. There are three that used to work that don't anymore: powertrain_binary, test_vga, and Gm1_v5_Practice

I'm looking to see if they have anything in common.

Some of these broke with the last PR merge. @kkunal1408 is looking into it now.

This is the current list:

FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-SW_Cres_v3_5]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-Sanitized_Coarse_Comp_CK]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-COMP_GM_STAGE_0415]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-powertrain_binary]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-telescopic_ota_guard_ring]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-vco_dtype_12_hierarchical]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-test_vga]
FAILED tests/integration/test_all.py::test_A[20-FinFET14nm_Mock_PDK-vco_dtype_12_hierarchical_res]

@stevenmburns
Copy link
Collaborator Author

stevenmburns commented Apr 30, 2021

I guess I'll keep looking into: Gm1_v5_Practice
This seems to be the only one that is failing in addition to what is failing on master now.

@stevenmburns stevenmburns changed the title WIP: Generate primitives with multiple aspect ratios and then use the placer to decide which to choose. Generate primitives with multiple aspect ratios and then use the placer to decide which to choose. Apr 30, 2021
@stevenmburns
Copy link
Collaborator Author

@parijatm @854768750 This is ready to check in.
There is some cleanup that should be done. Right now there are some warts in align/main.py that generate more primitives. We need to generate them in some better way. Also verilog file modification are being done there instead of at the source (the topology flow.) We can either address them now or in a subsequent PR.

@stevenmburns
Copy link
Collaborator Author

Here are two placement from current_mirror_ota where the leaf blocks have different aspect ratios
Screenshot from 2021-04-30 11-50-47
Screenshot from 2021-04-30 11-53-09

@@ -33,6 +36,59 @@ def build_steps( flow_start, flow_stop):
return steps_to_run


def gen_more_primitives( primitives, topology_dir, subckt):
Copy link
Collaborator Author

@stevenmburns stevenmburns Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkunal1408 @arvuce22 This is the code that needs to be moved upstream in the flow, either topology, or primitives or both.

@stevenmburns
Copy link
Collaborator Author

@parijatm Ready for review.

Copy link
Collaborator

@parijatm parijatm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of great changes in this PR.

Looks good to go !

@stevenmburns stevenmburns merged commit 9947659 into master Apr 30, 2021
@stevenmburns stevenmburns deleted the feature/one_to_many_map_file branch April 30, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants