global_placer fails with [ERROR GPL-0303] after timing driven call

[gadfort]

Describe the bug

global place is failing with [ERROR GPL-0303] Use a higher -density or re-floorplan with a larger core area. after the call to resizer. Seems like the density should have been adjusted afterwards.

Expected Behavior

It works

Environment

OpenROAD v2.0-17308-g45d014ffe 
Features included (+) or not (-): +Charts +GPU +GUI +Python
This program is licensed under the BSD-3 license. See the LICENSE file for details.
Components of this program may be licensed under more restrictive licenses which must be honored.

To Reproduce

sc_issue_aes_job0_place0_20241125-191910.tar.gz

tar xvf sc_issue_aes_job0_place0_20241125-191910.tar.gz
cd sc_issue_aes_job0_place0_20241125-191910
./run.sh

Relevant log output

[INFO GPL-0100] Timing-driven iteration 3/6, virtual: true.
[INFO GPL-0101] Iter: 284, overflow: 0.482, keep rsz at: 0.3
[INFO GPL-0106] Timing-driven: worst slack 7.04e-09
[INFO GPL-0103] Timing-driven: weighted 1178 nets.
[NesterovSolve] Iter:  290 overflow: 0.463 HPWL: 207496522
[NesterovSolve] Iter:  300 overflow: 0.421 HPWL: 215491281
[NesterovSolve] Iter:  310 overflow: 0.376 HPWL: 224207394
[NesterovSolve] Iter:  320 overflow: 0.325 HPWL: 232948087
[INFO GPL-0075] Routability numCall: 1 inflationIterCnt: 1 bloatIterCnt: 0
[INFO GPL-0036] TileBBox: (    0    0 ) ( 4200 4200 ) DBU
[INFO GPL-0038] TileCnt:      95   95
[INFO GPL-0040] NumTiles: 9025
[INFO GPL-0081] TotalRouteOverflow: 0.15779876708984375
[INFO GPL-0082] OverflowTileCnt: 4
[INFO GPL-0083] 0.5%RC: 0.9418743369371995
[INFO GPL-0084] 1.0%RC: 0.9171913416831048
[INFO GPL-0085] 2.0%RC: 0.891139480290492
[INFO GPL-0086] 5.0%RC: 0.848905217199199
[INFO GPL-0087] FinalRC: 0.9295328
[INFO GPL-0077] FinalRC lower than targetRC(1.01), routability not needed.
[INFO GPL-0100] Timing-driven iteration 4/6, virtual: false.
[INFO GPL-0101] Iter: 328, overflow: 0.283, keep rsz at: 0.3
[INFO GPL-0106] Timing-driven: worst slack 6.96e-09
[INFO GPL-0103] Timing-driven: weighted 1178 nets.
[INFO GPL-0107] Timing-driven: RSZ delta area:     2858.702
[INFO GPL-0108] Timing-driven: new target density: 0.5895949
[ERROR GPL-0303] Use a higher -density or re-floorplan with a larger core area.
Given target density: 0.59
Suggested target density: 0.60
Error: sc_place.tcl, 30 GPL-0303

Screenshots

No response

Additional Context

No response

[maliberty]

@gadfort is this a new failure today? We just merged a gpl change.

[gudeh]

I noticed that, since we don't have any area checking during rsz repair design, it might increase the area too much, to a point where it is impossible to achieve the desired density.

[gadfort]

@maliberty this includes the change in gpl. @gudeh It seems to me that you would need to modify the target density to account for the new and resized instances.

[gudeh]

I did so based on what routability-mode already used to do after changing instances areas. I am going to check that.

[gudeh]

@gadfort does this issue happen on other of your designs also? Would raising the density be an option? Also, am I able to use debug mode and the gui using SC scripts?

[gadfort]

@gudeh I don't think the right solution is to require changing the density. That would imply a user should know how much the resizer is going to add which would require us to artificially lower or raise the density from the desired.

You can just edit the build/aes/job0/place/0/replay.sh file to add the -gui flag (you might also need to remove the QT_QPA_PLATFORM env var if its there. As for debug mode, you might need to add something to build/aes/job0/sc_collected_files/scripts_47796b273299036fb7ff7ec933bb5669cfd57b7c/sc_place.sh to enable whatever you need.

This is the only design with this issue (that is in our CI), but it used to work just fine and I don't think this change should require configuration changes.

[maliberty]

@gadfort even before this change the user has to pick an achievable density which somehow considers what additional area will be generated through the flow. I don't see that as a new requirement. We will look at this issue and see if we can avoid the area increase.

[gadfort]

@maliberty I guess my point is it was successful through the flow before, so the density is fine

[gadfort]

I think it's just a calculation error: The requested density is 50%, this is increased automatically to 58.95%, but something else in the code things 60% is the minimum required.

[INFO GPL-0108] Timing-driven: new target density: 0.5895949

[maliberty]

@maliberty I guess my point is it was successful through the flow before, so the density is fine

My point is that it often occurs that an acceptable change improves many but not all designs. The requirement isn't monotonic improvement across all possible designs. It possible that this is in the unfortunate bucket. However this comment may not apply here.

[gudeh]

This is where the 60% comes from (uniformTargetDensity_), it is more of a "naive" calculation. This code detects that it is impossible to achieve the recently adjusted targetDensity because the movable area is smaller than the total instances area.

If we used a higher density, we would have more movable area.

[gudeh]

I guess my point is it was successful through the flow before, so the density is fine

I believe this statement is not exactly correct. What used to happen is that RSZ repair design used to do its modifications after gpl, completely disregarding density or any other important metric for GPL. We even used to have placement density hotspots because of that.

What happens now is that we do update the GPL internal density after area modifications made by RSZ. The density update follows the same steps as routability, although the area changes by routability are a lot smaller than the ones made by RSZ.

What I would like to be able to do is to modify the density update to take into account this big change in area, possibly automatically increasing the density. Do you think this is a good thing to try @maliberty ?

[maliberty]

It sounds reasonable. The density set previously for gpl didn't include the effect of repair_design and now it does.

[gadfort]

@maliberty I think @gudeh is referring to the "filler cells" that GPL uses, not actual filler cells.

[gudeh]

I realized something not directly related to the present issue: The calculation of total filler gcell area is done separately from the actual filler gcells existing objects. We consider a certain total filler gcell area for the density calculation, but their real area along algorithm is different. Since routability does not change the area as much, it seems this difference is ignored. Since the rsz makes larger changes, it could be relevant to implement a fix for that and analyze the results, but it is something that could take some time to do.

Considering Peter's issue itself, I believe the best approach would be to just use the uniform density (the suggested value) instead of throwing an error, since gpl completes with it. Worst case scenario with this change, for other designs falling under this situation (negative total filler gcell area), is to not converge and do the 5000 maximum iterations.

[gudeh]

Well I realized a small but important bug. I was not considering the change in area for newly instantiated cells, only for resized ones. Meaning we were not properly updating the target density, and using an incorrect change in area. I started a secure-CI with the fix.

[rovinski]

Related to my comment in https://github.com/The-OpenROAD-Project/OpenROAD/pull/6257#discussion_r1863087618 most commercial tools automatically choose a density based on the available core area. OR exposes this as a manual knob, hence the confusion about "is this unexpected to the user", but I'm not even sure if commercial tools even expose this knob. It would be nice to eventually move to automatically selecting the GPL density so that users don't have to worry about any of this - it just needs to fit in the core area and eventually be routable.

[maliberty]

@rovinski agreed but if it were easy to do it would have happened already. It is a crutch we eventually need to retire.