DPL negotiation default algorithm#4186
Open
openroad-ci wants to merge 27 commits into
Open
Conversation
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
replace USE_NEGOTIATION for DPL_USE_OLD_DIAMOND, since the OR submodule now does not have -use_negotiation anymore for dpl, and has -use_old_diamond, the default is negotiation legalizer Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the OpenROAD subproject, reduces core utilization for several designs, and renames the USE_NEGOTIATION variable to DPL_USE_OLD_DIAMOND to reflect that the negotiation legalizer is now the default. Feedback suggests removing leftover debug code in global_route.tcl and adding the stages field to the new variable definition in the configuration files for improved documentation and consistency.
Comment on lines
67
to
69
| #set_debug_level DPL negotiation 1 | ||
| dpl::detailed_placement_debug | ||
| log_cmd detailed_placement {*}$dpl_args |
There was a problem hiding this comment.
These lines appear to be leftover debug code. Commented-out code and explicit calls to dpl::detailed_placement_debug should be removed to keep the scripts clean and consistent with other stages (like CTS) that also perform detailed placement.
log_cmd detailed_placement {*}$dpl_args
Comment on lines
1433
to
1436
| "DPL_USE_OLD_DIAMOND": { | ||
| "default": 0, | ||
| "description": "Enable using negotiation legalizer for detailed placement.\n" | ||
| "description": "Use the former diamond search legalizer for detailed placement instead of the default negotiation legalizer.\n" | ||
| }, |
There was a problem hiding this comment.
It is recommended to include the stages field for the new DPL_USE_OLD_DIAMOND variable to document where it is used (place, cts, and grt) and to maintain consistency with other variables in this file.
"DPL_USE_OLD_DIAMOND": {
"default": 0,
"description": "Use the former diamond search legalizer for detailed placement instead of the default negotiation legalizer.\n",
"stages": [
"place",
"cts",
"grt"
]
},Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
===================================================== make update_ok for ibex (gf180)... ===================================================== designs/gf180/ibex/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | finish__timing__setup__tns | -2.22 | -4.76 | Failing | | finish__design__instance__area | 764974 | 764104 | Tighten | ===================================================== make update_ok for gcd (asap7)... ===================================================== designs/asap7/gcd/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | globalroute__timing__setup__tns | -1790.0 | -2050.0 | Failing | | detailedroute__route__wirelength | 1324 | 1947 | Failing | | finish__timing__setup__tns | -1570.0 | -1820.0 | Failing | ===================================================== make update_ok for riscv32i-mock-sram (asap7)... ===================================================== designs/asap7/riscv32i-mock-sram/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | globalroute__timing__setup__ws | -56.5 | -47.5 | Tighten | | globalroute__timing__setup__tns | -407.0 | -190.0 | Tighten | | finish__timing__setup__tns | -298.0 | -1330.0 | Failing | ===================================================== make update_ok for ibex (asap7)... ===================================================== designs/asap7/ibex/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -1630.0 | -13700.0 | Failing | | finish__design__instance__area | 2816 | 2810 | Tighten | ===================================================== make update_ok for uart (asap7)... ===================================================== designs/asap7/uart/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | detailedroute__route__wirelength | 1673 | 1973 | Failing | | finish__timing__setup__ws | -52.4 | -47.3 | Tighten | | finish__timing__setup__tns | -1320.0 | -1210.0 | Tighten | ===================================================== make update_ok for gcd-ccs (asap7)... ===================================================== designs/asap7/gcd-ccs/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__ws | -63.5 | -63.4 | Tighten | | cts__timing__setup__tns | -773.0 | -768.0 | Tighten | | globalroute__timing__setup__ws | -63.5 | -63.4 | Tighten | | globalroute__timing__setup__tns | -771.0 | -766.0 | Tighten | | finish__timing__setup__tns | -1270.0 | -1430.0 | Failing | ===================================================== make update_ok for chameleon (sky130hd)... ===================================================== designs/sky130hd/chameleon/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__ws | -0.962 | -0.945 | Tighten | | cts__timing__setup__tns | -6.72 | -8.5 | Failing | | globalroute__antenna_diodes_count | 196 | 194 | Tighten | | globalroute__timing__setup__ws | -0.944 | -0.909 | Tighten | | globalroute__timing__setup__tns | -5.91 | -5.68 | Tighten | | finish__timing__setup__ws | -0.916 | -0.906 | Tighten | | finish__timing__setup__tns | -7.59 | -6.54 | Tighten | ===================================================== make update_ok for gcd (sky130hd)... ===================================================== designs/sky130hd/gcd/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -68.8 | -69.9 | Failing | | globalroute__timing__setup__ws | -1.9 | -2.25 | Failing | | globalroute__timing__setup__tns | -88.8 | -93.7 | Failing | | detailedroute__route__wirelength | 14932 | 17828 | Failing | | finish__timing__setup__ws | -1.74 | -1.86 | Failing | | finish__timing__setup__tns | -81.2 | -79.8 | Tighten | ===================================================== make update_ok for microwatt (sky130hd)... ===================================================== designs/sky130hd/microwatt/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -345.0 | -335.0 | Tighten | | globalroute__antenna_diodes_count | 1426 | 1421 | Tighten | | globalroute__timing__setup__tns | -322.0 | -288.0 | Tighten | | detailedroute__antenna__violating__nets | 5 | 1 | Tighten | | detailedroute__antenna_diodes_count | 1451 | 1477 | Failing | | finish__timing__setup__tns | -158.0 | -140.0 | Tighten | | finish__design__instance__area | 5572106 | 5571256 | Tighten | ===================================================== make update_ok for aes (sky130hd)... ===================================================== designs/sky130hd/aes/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | globalroute__timing__setup__ws | -1.3 | -0.523 | Tighten | | globalroute__timing__setup__tns | -9.57 | -12.5 | Failing | | finish__timing__setup__ws | -1.17 | -0.461 | Tighten | | finish__timing__setup__tns | -2.8 | -4.58 | Failing | ===================================================== make update_ok for gcd (sky130hs)... ===================================================== designs/sky130hs/gcd/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -11.4 | -12.3 | Failing | | globalroute__timing__setup__tns | -19.2 | -20.3 | Failing | | detailedroute__route__wirelength | 14238 | 16488 | Failing | | finish__timing__setup__tns | -15.8 | -17.2 | Failing | ===================================================== make update_ok for aes (sky130hs)... ===================================================== designs/sky130hs/aes/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | placeopt__design__instance__area | 160499 | 159595 | Tighten | | placeopt__design__instance__count__stdcell | 19517 | 19233 | Tighten | | globalroute__timing__setup__tns | -1.92 | -3.16 | Failing | | finish__design__instance__area | 176489 | 174570 | Tighten | ===================================================== make update_ok for swerv (nangate45)... ===================================================== designs/nangate45/swerv/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | synth__design__instance__area__stdcell | 178043.59 | 166000.0 | Tighten | | placeopt__design__instance__area | 179149 | 177861 | Tighten | | placeopt__design__instance__count__stdcell | 99342 | 98391 | Tighten | | cts__design__instance__count__setup_buffer | 8638 | 8556 | Tighten | | cts__design__instance__count__hold_buffer | 8638 | 8556 | Tighten | | cts__timing__setup__tns | -363.0 | -398.0 | Failing | | globalroute__antenna_diodes_count | 102 | 101 | Tighten | | globalroute__timing__setup__tns | -420.0 | -498.0 | Failing | | detailedroute__route__wirelength | 2659376 | 2435536 | Tighten | | finish__timing__setup__tns | -396.0 | -438.0 | Failing | ===================================================== make update_ok for bp_fe_top (nangate45)... ===================================================== designs/nangate45/bp_fe_top/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | finish__timing__setup__ws | -0.15 | -0.14 | Tighten | | finish__timing__setup__tns | -1.58 | -1.95 | Failing | ===================================================== make update_ok for jpeg (nangate45)... ===================================================== designs/nangate45/jpeg/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | synth__design__instance__area__stdcell | 102000.0 | 99800.0 | Tighten | | placeopt__design__instance__count__stdcell | 68509 | 68139 | Tighten | | cts__design__instance__count__setup_buffer | 5957 | 5925 | Tighten | | cts__design__instance__count__hold_buffer | 5957 | 5925 | Tighten | | cts__timing__setup__tns | -54.6 | -43.0 | Tighten | | globalroute__timing__setup__tns | -66.5 | -52.7 | Tighten | | detailedroute__route__wirelength | 631144 | 767282 | Failing | | finish__timing__setup__tns | -53.3 | -46.7 | Tighten | ===================================================== make update_ok for tinyRocket (nangate45)... ===================================================== designs/nangate45/tinyRocket/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -30.0 | -37.4 | Failing | | globalroute__timing__setup__tns | -50.3 | -64.7 | Failing | | finish__timing__setup__tns | -42.8 | -57.6 | Failing | ===================================================== make update_ok for mempool_group (nangate45)... ===================================================== designs/nangate45/mempool_group/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | cts__timing__setup__tns | -12000.0 | -12100.0 | Failing | | globalroute__timing__setup__tns | -14000.0 | -12100.0 | Tighten | ===================================================== make update_ok for ariane133 (nangate45)... ===================================================== designs/nangate45/ariane133/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | placeopt__design__instance__area | 827643 | 827361 | Tighten | | cts__timing__setup__tns | -502.0 | -556.0 | Failing | | globalroute__timing__setup__tns | -556.0 | -642.0 | Failing | | finish__timing__setup__tns | -549.0 | -604.0 | Failing | | finish__design__instance__area | 837050 | 836289 | Tighten | ===================================================== make update_ok for spi (ihp-sg13g2)... ===================================================== designs/ihp-sg13g2/spi/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | detailedroute__route__wirelength | 3972 | 4623 | Failing | ===================================================== make update_ok for i2c-gpio-expander (ihp-sg13g2)... ===================================================== designs/ihp-sg13g2/i2c-gpio-expander/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | detailedroute__route__wirelength | 37489 | 44583 | Failing | ===================================================== make update_ok for ibex (ihp-sg13g2)... ===================================================== designs/ihp-sg13g2/ibex/rules-base.json updates: | Metric | Old | New | Type | | ------ | --- | --- | ---- | | synth__design__instance__area__stdcell | 280000.0 | 278000.0 | Tighten | | placeopt__design__instance__count__stdcell | 20659 | 20256 | Tighten | | detailedroute__route__wirelength | 895142 | 1137972 | Failing | Large percentage changes in failing metrics (>50%): - ibex (asap7) cts__timing__setup__tns 740.49% (-1630.0 → -13700.0) - riscv32i-mock-sram (asap7) finish__timing__setup__tns 346.31% (-298.0 → -1330.0) - ibex (gf180) finish__timing__setup__tns 114.41% (-2.22 → -4.76) - aes (sky130hs) globalroute__timing__setup__tns 64.58% (-1.92 → -3.16) - aes (sky130hd) finish__timing__setup__tns 63.57% (-2.8 → -4.58) Large percentage improvements in tighten metrics (>50%): - microwatt (sky130hd) detailedroute__antenna__violating__nets -80.00% (5 → 1) - aes (sky130hd) finish__timing__setup__ws -60.60% (-1.17 → -0.461) - aes (sky130hd) globalroute__timing__setup__ws -59.77% (-1.3 → -0.523) - riscv32i-mock-sram (asap7) globalroute__timing__setup__tns -53.32% (-407.0 → -190.0) Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
openroad-ci
force-pushed
the
secure-dpl-negotiation-default
branch
from
f282ea5 to
75c8d96
Compare
Contributor
|
All green. Ignore the updates from the bot, they were wrong, I did the updates manually |
Member
Please update the description removing the old wrong reports, and if possible add the reports from your manual update. |
Contributor
|
I don't seem to be able to edit the bot's initial message. I added the updates on the commit message. I am repeating them here: |
Member
|
These two failures seem quite large, specially the one in IMO it's worth to understand what placement changes caused these large degradations. |
Contributor
|
I loaded asap7/ibex during CTS for both negotiation and with master. and with negotiation: Rolling over the list of paths I noticed with negotiation we get some paths with different required times, and they also have higher skew. Notice rolling to the end of the list for negotiation: While with master all the paths in the list have 0 skew. Also showing the last item on the list. |
Member
|
how do the slack histograms compare? |
Contributor
master by the left:
|
Member
|
A lot of endpoints go from passing to failing. |
Contributor
|
The following should be the same path for both (or at least really similar one). Notice the negative slack for negotiation, and positive for master. Negotiation: Master: |
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Contributor
|
On negotiation branch, we get a fanout of 30 with this net in green: while on master a potion of this net was broken into two: The buffer named |
Contributor
|
At stage 3-4 and stage 3-5 the green net has 30 fanouts on both master and negotiation. I understand the issue is downstream at CTS optimizations. |
Member
Is the green net in the critical path? If so, something in the final detailed placement from negotiation-based makes CTS to understand there is no issues with this net, skipping the buffer insertion. You can check if, in master, the buffer is inserted during CTS or in the post-CTS repair_timing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates metrics for OR PR The-OpenROAD-Project/OpenROAD#10226 to make new negotiation legalizer algorithm into the default one.
This PR also replaces USE_NEGOTIATION for DPL_USE_OLD_DIAMOND, since the OR submodule now does not have
-use_negotiationanymore for dpl, and has-use_old_diamond.I also modified the utilization of a few failing designs.