psm: error early when a report writer has no solution, and skip unpowered nodes - #11103
Conversation
…ered nodes The three writers derive their output from a solution and all reached into currents_/voltages_ with at(), so calling one before analyze_power_grid threw a bare std::out_of_range with nothing to act on. write_pg_spice threw after it had already written the resistive network, leaving a syntactically valid spice deck with no sources and no sinks -- a floating network a simulator solves to nonsense rather than rejects. Each writer now calls ensureSolution() before creating its file, which reports the net and corner and names analyze_power_grid. writeSpiceFile additionally looked up every ITermNode in the per-node current map with at(). That map is sparse by construction: generateCurrentMap() inserts only nodes of instances returned by getInstancePower(), so a filler, tap or decap has no entry. solve() reads the same map with find() and a zero default, so the sparse case was already handled twenty lines of context away. The lookup now matches, and skips a node with no current the way the neighbouring kSpiceFileMinCurrent guard already skips a negligible one. Adds report_writers_require_solution, covering write_pg_spice before a solution exists -- including that no file is left behind -- and all three writers after one. Fixes The-OpenROAD-Project#11098 Signed-off-by: Shivaram Mysore <shivaram.mysore@gmail.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 introduces safety checks in the IRSolver class to ensure a power grid solution exists before generating reports (voltage, EM, and SPICE files). Specifically, a new ensureSolution method is added and called in the report-writing functions to prevent out-of-range map lookups and output clear error messages. Additionally, writeSpiceFile is updated to safely handle instances that do not draw power by using find instead of at on the currents map. A new integration test has been added to verify this behavior. There are no review comments, so I have no feedback to provide.
The ensureSolution comment ran to 81 columns against the 80-column limit. Verified with the same clang-format CI uses, Ubuntu 18.1.3, via --dry-run -Werror on both changed files. Signed-off-by: Shivaram Mysore <shivaram.mysore@gmail.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b8b59189e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| insert_decap_with_padding1 | ||
| missing_resistance | ||
| pad_connected_by_abutment | ||
| report_writers_require_solution |
There was a problem hiding this comment.
When PSM tests are run through Bazel, this CMake-only addition never creates a report_writers_require_solution target because src/psm/test/BUILD generates targets exclusively from ALL_TESTS, where the new name is absent. Consequently, Bazel CI cannot execute this regression; add it to the Bazel test list as well.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
| if (hasSolution(corner)) { | ||
| return; |
There was a problem hiding this comment.
When a grid is solved without any powered instances—for example, a DEF-only or early-floorplanning analysis using set_pdnsim_net_voltage and no Liberty-backed loads—solve() still populates the voltage solution, but currents_[corner] remains empty, so hasSolution() returns false here. This newly makes analyze_power_grid -voltage_file ..., EM output, and write_pg_spice report “Run analyze_power_grid first” immediately after a successful solve; the writer guard should check the data each writer actually requires rather than treating an empty load map as no solution.
Useful? React with 👍 / 👎.
Addresses both review comments on The-OpenROAD-Project#11103. hasSolution() requires the current map to be non-empty, so a grid solved with no powered instances -- a DEF-only or floorplanning analysis, where the solve populates voltages and generateCurrentMap() inserts nothing -- would have been rejected by the guard immediately after a successful solve. Voltages are still reportable in that case. Each writer now tests the map it actually reads: the voltage and EM writers require node voltages, and the spice writer requires the current map, whose emptiness is already handled by the find() lookup over ITerm nodes. The helper is renamed reportNoSolution() since it no longer decides, only reports. Also registers report_writers_require_solution in src/psm/test/BUILD. It was added to the CMake list only, so Bazel generated no target for it and Bazel CI could not run the regression. AGENTS.md asks for dual CMake+Bazel registration. Signed-off-by: Shivaram Mysore <shivaram.mysore@gmail.com>
|
Both addressed in Accept solved grids with an empty current map (P2). Correct, and the reasoning is right: Each writer now tests the map it actually reads rather than a global notion of "solved":
The helper is renamed Register with Bazel (P1). Also correct — One thing to flag rather than leave implicit: I could not run |
|
Follow-up on my formatting caveat: the machine came back, and I have now run the same |
3 participants
Fixes #11098, doing the three things @gadfort asked for.
Error before the file is created
writeSpiceFile,writeInstanceVoltageFileandwriteEMFileall reach intocurrents_/voltages_withat(), so calling one before a solution exists threw a barestd::out_of_rangewith nothing to act on. Each now calls a smallensureSolution()first — before opening its stream — reporting the net and corner and naminganalyze_power_grid:This matters most for
write_pg_spice, which threw after writing the resistive network: it left a syntactically valid deck with no sources and no sinks — a floating network a simulator solves to nonsense rather than rejects.find()for the sparse current mapwriteSpiceFilelooked up everyITermNodein the per-node current map withat(). That map is sparse by construction —generateCurrentMap()inserts only nodes of instances returned bygetInstancePower(), so a filler, tap or decap has no entry.solve()reads the same map withfind()and a zero default (ir_solver.cpp:932-937), twenty lines of context away, so the sparse case was already handled correctly there. The lookup now matches, and skips a node with no current the way the neighbouringkSpiceFileMinCurrentguard already skips a negligible one.Test
report_writers_require_solutioncoverswrite_pg_spicebefore a solution exists — asserting the error and that no file is left behind — thenanalyze_power_grid -voltage_file -enable_em -em_outfilefollowed bywrite_pg_spice, so all three writers are exercised on the success path too.Two things worth flagging
I could not run the suite locally, so the
.okwas composed rather than captured: the ODB banner frommissing_resistance.ok, the report block fromgcd_em_test_vdd.ok, andwrite_pg_spice's ownPSM-0015line fromgcd_write_sp_test_vdd.ok— all on the same design and invocation. It should be exact, but please have CI confirm it rather than taking my word.The sparse-map path is not covered by this test, and I would rather say so than imply it is. Nangate45's
FILLCELL_X1has no pins and appears in no net, so gcd creates no unpoweredITermNodeandat()never throws there — which is whygcd_write_sp_test_vddpasses today. Reproducing it needs a design whose unpowered instances connect to the power net, as sky130-style flows do via( * VPWR ); I hit it on a 72k-instance sky130 block where 57,142 of the instances carry no power. If you would like that covered,sky130hd_data/gcd_sky130hd_floorplan.deflooks like the right existing fixture and I am happy to add it in a follow-up.